Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add some fixture utils ... #151

Merged
merged 10 commits into from
Jul 17, 2023
Merged

Add some fixture utils ... #151

merged 10 commits into from
Jul 17, 2023

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented Jul 14, 2023

and assert that the .prettierrc.js is generated correctly for all tests, ensuring that the fixture utils work

These are util needed for finishing #146
and #153

An example of using these utils can be seen here: #154

Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if my comments rise to the level of "request changes" 🤷

tests/.eslintignore Outdated Show resolved Hide resolved
tests/utils.ts Outdated
scenario?: string;
}
) {
let scenario = options?.scenario ?? 'default';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that the default is the default value for scenario here, because it matters what folder you put the test file in. I feel like you could make this just a plain argument of the function and make it required, rather than part of the options and making it optional

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I don't find default being the default too surprising 🙃

tests/utils.ts Outdated Show resolved Hide resolved
tests/assertions.ts Show resolved Hide resolved
tests/assertions.ts Show resolved Hide resolved
Copy link
Collaborator

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, with a minor suggestion, and also pointing to this related comment.

Btw, if people are concerned by introducing too much of our own testing infrastructure, we could also introduce chai-files, which is also used in the blueprint tests of ember-cli/ember-source IIRC, and which vitest can utilize. Not sure though if this will help much, as it's concerned about file assertions, not so much about generating fixtures...

tests/utils.ts Outdated

export const SUPPORTED_PACKAGE_MANAGERS = ['npm', 'yarn', 'pnpm'] as const;

export async function fixture(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not sure what this is doing when seeing it used in code, like generating a fixture, asserting against one, or reading it (which is happening here). Maybe rename to readFixture?

tests/utils.ts Outdated
scenario?: string;
}
) {
let scenario = options?.scenario ?? 'default';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I don't find default being the default too surprising 🙃

@NullVoxPopuli NullVoxPopuli force-pushed the add-fixture-utils branch 2 times, most recently from e524cc5 to 0e99425 Compare July 17, 2023 12:10
…creation of addons

will need to propagate through the existing tests
but that can happen in other PRs
@NullVoxPopuli NullVoxPopuli force-pushed the add-fixture-utils branch 2 times, most recently from 055732f to 387847b Compare July 17, 2023 12:26
@NullVoxPopuli NullVoxPopuli merged commit 35e7ca0 into main Jul 17, 2023
@NullVoxPopuli NullVoxPopuli deleted the add-fixture-utils branch July 17, 2023 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants