feat(runner): add mergeTests utility to compose TestAPI fixtures#9662
feat(runner): add mergeTests utility to compose TestAPI fixtures#9662Ujjwaljain16 wants to merge 21 commits intovitest-dev:mainfrom
Conversation
- Adds resolveFixtures() to TestFixtures - Implements mergeTests(testA, testB) using existing .extend() mechanism - Supports override semantics (last writer wins) - Includes runtime tests and type-level tests - Verified nested describe, shared base, and chained merges
|
@sheremet-va waiting for the review |
packages/runner/src/fixture.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * @internal |
There was a problem hiding this comment.
Everything here is internal, so no need to add this
packages/runner/src/fixture.ts
Outdated
| /** | ||
| * @internal | ||
| */ | ||
| resolveFixtures(): UserFixtures { |
There was a problem hiding this comment.
I don't like this idea, why do we need to convert already converted fixtures into user definitions and then convert them back again?
I would do the opposite and add a merge function that accepts another Fixtures, then we can just iterate over the registrations overriding them
packages/runner/src/suite.ts
Outdated
| export function mergeTests<A, B>( | ||
| test: TestAPI<A>, | ||
| testB: TestAPI<B>, | ||
| ): TestAPI<A & B> { |
There was a problem hiding this comment.
Hm, I assumed it can accept more than 2 (any number in fact). Would it make sense?
There was a problem hiding this comment.
Agreed this utility should support composing any number of test APIs.
I updated the signature to:
mergeTests<T extends readonly unknown[]>(...tests: T)Runtime behavior:
Iteratively extends left -> right.
Later fixtures override earlier ones (last-writer-wins)
Works for arbitrarily long argument lists.
Type behavior:
MergeTestContexts<T> iterates over the tuple and intersects all extracted contexts.
Variadic inference is preserved even for nested merges.
This is now covered by runtime and type tests (including 3+ merges)
packages/runner/src/suite.ts
Outdated
| ): TestAPI<A & B> { | ||
| const ctx = getChainableContext(testB) | ||
| if (!ctx) { | ||
| throw new TypeError('Cannot merge tests: extension is not a valid TestAPI') |
There was a problem hiding this comment.
This is a bit cryptic, what is a TestAPI to the user? Maybe just mention that this function requires an extended test
There was a problem hiding this comment.
Agreed exposing internal terminology like "TestAPI" in runtime errors isn’t ideal
I updated the error message to:
"mergeTests requires extended test instances created via test.extend()"
This makes the requirement clearer without leaking internal implementation concepts
packages/vitest/src/public/index.ts
Outdated
| beforeEach, | ||
| describe, | ||
| it, | ||
| mergeTests, |
There was a problem hiding this comment.
This function needs to be documented
|
@sheremet-va
While implementing T extends readonly TestAPI<any>[]However this fails because <ExtraContext extends C>(...)This makes: TestAPI<{ a: number }>not assignable to: TestAPI<any>So constraining to To work around this, I relaxed the constraint to: T extends readonly unknown[]and enforced type validity structurally via This preserves full context inference for consumers while avoiding the invariance trap. My question: Would you prefer a stricter nominal constraint even if it requires internal casts, or is structural validation acceptable here given the variance characteristics of
I initially attempted: T extends TestAPI<infer C> ? C : neverBut this proved unreliable because Instead, I switched to structural extraction based on T extends { beforeEach: (fn: infer F, ...args: any) => any }
? F extends (context: infer C, ...args: any) => any
? C
: never
: neverThis also correctly accounts for the actual listener signature: (context, suite)Structurally matching on Would you prefer: Structural extraction (as implemented), or
I explored implementing a low-level However, that would: Bypass the Instead, I:
This keeps all validation inside the existing extension mechanism and preserves immutability. Would you consider this acceptable alignment with Vitest’s design, or would you prefer fixture merging to happen at the I’m happy to adjust direction based on your preference |
Casting types internally is fine to me as long as public API is strict
I don't have a preference. As long as public API works correctly, any trick is good to me
Why would it do that? Didn't we already validated everything in the original Could we abstract it in a better way, so it's more reusable without extra loops?
I don't know what that means |
I think one of the possible solutions here is to define a lot of overloads of function mergeTests<A, B, C>(test1: TestAPI<A>, test2: TestAPI<B>, test3: TestAPI<C>): TestAPI<A & B & C>
function mergeTests<A, B>(test1: TestAPI<A>, test2: TestAPI<B>): TestAPI<A & B>
// ... and so on |
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
4902540 to
d8e6beb
Compare
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
- Refactor mergeTests to use explicit nominal overloads (up to 6 args) - Remove complex structural extraction types (ExtractTestContext, UnionToIntersection) - Implement context-aware fixture merging in TestFixtures.merge - Add comprehensive runtime and type tests for mergeTests - Ensure strictly nominal TestAPI propagation
5878ad9 to
adf5044
Compare
091af14 to
a131cf9
Compare
57165f3 to
58c9f5b
Compare
sheremet-va
left a comment
There was a problem hiding this comment.
It's pretty hard to review
The previous implementation is clean and easy to follow, the new one removes all the helpful comments and introduces concepts and code that's just hard to follow
There are registrations and overrides. registrations are scoped to the last test.extend, and overrides are applied to tests in specified suites, that's it. What this PR adds are some kind of pure registrations, and keeps track of parents and ancestors (why? we have registrations already - overridden suites also include all fixtures already)
|
Appreciate the push to simplify you were right about the overhead. I've refactored the implementation back to the baseline 'flat model' and moved to direct Map-merging in extend() |
sheremet-va
left a comment
There was a problem hiding this comment.
I don't think there is enough tests. Unit tests are not enough, we need a comprehensive test suite similar to test/cli/test/scoped-fixtures.test.ts
What happens here, for example?
const t1 = test.extend('f', { scope: 'file' }, () => 'file')
const t2 = test.extend('f', { scope: 'test', () => 'test')
mergeTests(t1, t2)This should fail properly, for example
There are also no tests that have scope, injected, or even a function fixture (() => something)
| export function mergeTests<A, B, C, D, E, F>(a: TestAPI<A>, b: TestAPI<B>, c: TestAPI<C>, d: TestAPI<D>, e: TestAPI<E>, f: TestAPI<F>): TestAPI<A & B & C & D & E & F> | ||
| export function mergeTests(...tests: TestAPI<any>[]): TestAPI<any> { | ||
| if (tests.length === 0) { | ||
| throw new TypeError('mergeTests requires at least one test') |
| let [currentTest, ...rest] = tests | ||
|
|
||
| for (const nextTest of rest) { | ||
| const nextContext = getChainableContext(nextTest) |
| // Extract fixtures from the next test and extend the current test | ||
| // This behaves exactly like currentTest.extend(nextFixtures) | ||
| const currentContext = getChainableContext(currentTest) | ||
| if (!currentContext) { |
| return new TestFixtures(registrations) | ||
| } | ||
|
|
||
| getFixtures(): TestFixtures { |
There was a problem hiding this comment.
What is this? Why do you need this if you already have access to the instance?
Description
This adds a new
mergeTests(testA, testB)utility that allows composing fixtures from multipleTestAPIinstances.Example:
Implementation
The implementation reuses the existing
.extend()mechanism to preserve fixture graph integrity.Instead of manually merging internal
TestFixturesmaps, the approach is:testBviaresolveFixtures()testA.extend(fixturesFromB)TestAPI<A & B>This ensures:
Override Semantics
If both tests define the same fixture name, the second argument (
testB) overrides the first.This mirrors
.extend()behavior (last-writer-wins).Type Support
The return type is
TestAPI<A & B>.Type-level tests verify correct intersection inference and chained merging.
Tests
Added runtime tests covering:
describeAdded type-level tests verifying fixture intersection types.
All tests pass.
Resolves #9483
Tests
Run:
Documentation
No public docs changes required. The utility is self-contained and exposed via public API.
Changesets
PR name follows conventional format: