-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: support type inference with a new test.extend syntax
#9550
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
base: main
Are you sure you want to change the base?
feat: support type inference with a new test.extend syntax
#9550
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
✅ Deploy Preview for vitest-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
4f3ae6f to
af1c234
Compare
b2c0ea8 to
cf27bce
Compare
test.extend syntaxtest.extend syntax
hi-ogawa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API looks better to me than use magic. It still delegates to use mechanism internally, but I like it.
I haven't look what's going to happen in next PR #9553
45fe275 to
c3a889c
Compare
| }, { globals: true }) | ||
| expect(stdout).toContain('basic.test.ts') | ||
| expect(stderr).toBe('') | ||
| expect(stderr).toMatchInlineSnapshot(` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change, but it simplifies mental model A LOT. Scoped fixtures can only use other scoped fixtures with a higher scope, and scope is explicit (but overriding the fixture also inherits all the options by default, like in PW).
By default, any extended value has a test scope. You cannot override the extended value to provide a DIFFERENT scope, it now throws an error. This is needed to remove the ambiguity of defining a worker fixture and then overriding its "static" dependency inside the suite:
// this worked previously
const test = base
.extend('port', 5300)
.extend('config', { scope: 'worker' }, ({ port }) => {
return `localhost:${port}`
})But if you want to override the port, you need to invalidate or overwrite the WORKER fixture that we already initialized - which makes little sense
test.override('port', 5000) // what to do here?Using test.override with a scope other than test in general is now forbidden, unless it's called at the top level of the file (which still causes some unexpected issues with worker fixtures because they will leak to other test files in the same worker - I wonder if we could force a new worker instead).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also makes it so you cannot call test.extend('', { scope: 'file' }) inside a suite, it's only allowed in the top level of the module now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't checking these test cases previously and the new extend/override behavior makes sense to me.
|
@hi-ogawa I updated the fixtures inheritance (there is now a |
c3a889c to
225e631
Compare
| >> db fixture teardown | ||
| >> aroundEach teardown" | ||
| >> aroundEach teardown | ||
| >> db fixture teardown" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This behaviour seems to make more sense than what was here before because you should be able to access db after calling runTest(). Previously the db was torn down by that point 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this looks like what I suggested to happen in #9450 (comment) I forgot to double check the test.
8dc09a8 to
3d73ede
Compare
3d73ede to
a85eea4
Compare
|
Damn it, snapshots fail on rolldown because it processes files differently 😡 |
|
|
||
| By default, fixtures are initialized for each test. You can change this with the `scope` option to share fixtures across tests. | ||
|
|
||
| ::: warning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this warning section, @hi-ogawa. Would love to know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior looks reasonable to me. Note sure "force a new worker" behavior is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "force a new worker" is what playwright does, otherwise worker fixtures ovrerride each other in different files
hi-ogawa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestFixtures class looks great.
packages/runner/src/map.ts
Outdated
| } | ||
|
|
||
| export function getTestFixture<Context = TestContext>(key: Context): FixtureItem[] { | ||
| export function getTestFixturesManager<Context = TestContext>(key: Context): TestFixtures { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: any reason renaming only getTestFixture -> getTestFixturesManager but keeping setTestFixture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was one of the old attempts, I reverted to TestFixtures. There are now fixtures (the instance of the class) that have registrations
| } | ||
|
|
||
| extend(runner: VitestRunner, userFixtures: Record<string, any>): TestFixtures { | ||
| const { suite } = getCurrentSuite() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: TestFixtures structure looks great, but TestFixtures.extend/override relying on getCurrentSuite feels odd in terms of state separation. How about pass it from argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did pass it before, but moved it into the function directly in the last commit. We already call this function inside withFixtures anyway. The suite collection is intentionally global and fixture values rely on what suite they are in, so they are inherently tied. If we had a separate Runner class that does the collection, then it would make sense
| >> db fixture teardown | ||
| >> aroundEach teardown" | ||
| >> aroundEach teardown | ||
| >> db fixture teardown" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this looks like what I suggested to happen in #9450 (comment) I forgot to double check the test.
| }, { globals: true }) | ||
| expect(stdout).toContain('basic.test.ts') | ||
| expect(stderr).toBe('') | ||
| expect(stderr).toMatchInlineSnapshot(` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't checking these test cases previously and the new extend/override behavior makes sense to me.

Description
TODO
This PR adds new
test.extendsyntax that supports type inference:Fixes #9305
Fixes #9555
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yamlunless you introduce a new test example.Tests
pnpm test:ci.Documentation
pnpm run docscommand.Changesets
feat:,fix:,perf:,docs:, orchore:.