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

fix(runner): fix fixture cleanup when test times out #4679

Merged
merged 14 commits into from
Dec 7, 2023
62 changes: 25 additions & 37 deletions packages/runner/src/fixture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,49 +78,37 @@ export function withFixtures(fn: Function, testContext?: TestContext) {
if (!pendingFixtures.length)
return fn(context)

let cursor = 0

return new Promise((resolve, reject) => {
async function use(fixtureValue: any) {
const fixture = pendingFixtures[cursor++]
context![fixture.prop] = fixtureValue

if (!fixtureValueMap.has(fixture)) {
fixtureValueMap.set(fixture, fixtureValue)
cleanupFnArray.unshift(() => {
fixtureValueMap.delete(fixture)
async function resolveFixtures() {
for (const fixture of pendingFixtures) {
// fixture could be already initialized during "before" hook
if (fixtureValueMap.has(fixture))
continue

let fixtureValue: unknown
if (fixture.isFn) {
// wait for `use` call to extract fixture value
fixtureValue = await new Promise((resolveUseArg, rejectUseArg) => {
fixture.value(context, (useArg: unknown) => {
resolveUseArg(useArg)
// suspend fixture function until cleanup
return new Promise<void>((resolveUseReturn) => {
cleanupFnArray.push(resolveUseReturn)
})
}).catch(rejectUseArg) // treat fixture function error (before `use` call) as test failure
})
}

if (cursor < pendingFixtures.length) {
await next()
}
else {
// When all fixtures setup, call the test function
try {
resolve(await fn(context))
}
catch (err) {
reject(err)
}
return new Promise<void>((resolve) => {
cleanupFnArray.push(resolve)
})
fixtureValue = fixture.value
}
context![fixture.prop] = fixtureValue
fixtureValueMap.set(fixture, fixtureValue)
cleanupFnArray.unshift(() => {
fixtureValueMap.delete(fixture)
})
}
}

async function next() {
const fixture = pendingFixtures[cursor]
const { isFn, value } = fixture
if (fixtureValueMap.has(fixture))
return use(fixtureValueMap.get(fixture))
else
return isFn ? value(context, use) : use(value)
}

const setupFixturePromise = next().catch(reject)
cleanupFnArray.unshift(() => setupFixturePromise)
Comment on lines -121 to -122
Copy link
Contributor Author

@hi-ogawa hi-ogawa Dec 6, 2023

Choose a reason for hiding this comment

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

Maybe the issue could be simply this and removing these lines might fix #4669 but I suspect it has some reason to await this promise?

})
return resolveFixtures().then(() => fn(context))
}
}

Expand Down
3 changes: 1 addition & 2 deletions test/fails/fixtures/test-extend/fixture-error.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@ describe('error thrown in test fixtures', () => {
myTest('fixture errors', ({ a }) => {})
})

// TODO: enable when #4669 is fixed
describe.skip('correctly fails when test times out', () => {
describe('correctly fails when test times out', () => {
const myTest = test.extend<{ a: number }>({
a: async ({}, use) => {
await use(2)
Expand Down
3 changes: 2 additions & 1 deletion test/fails/test/__snapshots__/runner.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ TypeError: failure"
exports[`should fail test-extend/circular-dependency.test.ts > test-extend/circular-dependency.test.ts 1`] = `"Error: Circular fixture dependency detected: a <- b <- a"`;

exports[`should fail test-extend/fixture-error.test.ts > test-extend/fixture-error.test.ts 1`] = `
"Error: Error thrown in test fixture
"Error: Test timed out in 20ms.
Error: Error thrown in test fixture
Error: Error thrown in afterEach fixture
Error: Error thrown in beforeEach fixture"
`;
Expand Down