[Regression]: If fixture timeouts in teardown phase, no other fixtures run

[vitalets]

Last Good Version

1.42

First Bad Version

1.43

Steps to reproduce

Please check out the README in https://github.com/vitalets/playwright-issues/tree/timeout-in-fuxtures-1-43

Expected behavior

I expect all fixtures run teardown phase in case of timeout.

Actual behavior

No fixtures run teardown phase after timeout.

Additional context

No response

Environment

System:
    OS: macOS 13.4.1
    CPU: (8) arm64 Apple M1
    Memory: 96.66 MB / 16.00 GB
  Binaries:
    Node: 18.16.0 - ~/.nvm/versions/node/v18.16.0/bin/node
    Yarn: 1.22.21 - ~/.nvm/versions/node/v18.16.0/bin/yarn
    npm: 9.5.1 - ~/.nvm/versions/node/v18.16.0/bin/npm
    pnpm: 7.27.1 - ~/Library/pnpm/pnpm
  IDEs:
    VSCode: 1.87.2 - /usr/local/bin/code
  Languages:
    Bash: 3.2.57 - /bin/bash
  npmPackages:
    @playwright/test: 1.43.0-beta-1711653598000 => 1.43.0-beta-1711653598000

[dgozman]

@vitalets Thank you for the issue!

In general, this is the intended behavior. Once a fixture times out, we run out of time budget to teardown other fixtures. Previously, this worked in a limited number of scenarios, where we would teardown the worker and give it some more time to teardown the fixtures. This only worked when the first fixture won't stall forever, but actually finish given a bit more time, which makes it even less useful. Also, this does not play nicely with things like afterAll() that need all fixtures to be teared down before they can run, and so we cannot introduce extra timeouts there.

It would be nice to understand your usecase - why do you rely on the second fixture teardown to be executed? Perhaps we can improve the situation a bit when there are no hooks, similarly to the old behavior. However, the second timeout will definitely prevent anything else from running.

[vitalets]

@dgozman thanks for the explanation!

My use-case is for playwright-bdd project: after running all user-defined fixtures I always need to attach some metadata to the testInfo. This is needed for the correct reporting, even after timeout.

I've also tried to provide a timeout option for my fixture, but it does not help:

export const test = base.extend<{timeoutedFixture: void, anotherFixture: void}>({
  timeoutedFixture: async ({anotherFixture}, use, testInfo) => {
    await use();
    await new Promise((r) => setTimeout(r, testInfo.timeout + 100));
  },
  anotherFixture: [async ({}, use) => {
    console.log('AnotherFixture: setup');
    await use();
    console.log('AnotherFixture: teardown'); // <-- I expect this to run, b/c fixture has own timeout and should not care about other fixtures timeouts
  }, { timeout: 10000 }],
});

Now I'm thinking about two possible solutions:

1. perform empty attachment at the setup phase, and then modify this attachment in-place. Is it reliable enough?
2. have some synchronous way to pass data to the reporter, see https://github.com/microsoft/playwright/issues/30179

For the more common case I think this could be a trade-off: for some users time budget is more important, but for others it's more important to cleanup fixtures that already passed the setup phase, even if it exceeds the timeout. Maybe there can be an option for that?

[dgozman]

I've also tried to provide a timeout option for my fixture, but it does not help

This is an interesting idea! Having a separate timeout would resolve concerns about running out of the time budget. I'll take a stab at implementing this.

but for others it's more important to cleanup fixtures that already passed the setup phase, even if it exceeds the timeout. Maybe there can be an option for that?

Yeah, I think the custom fixture timeout is a good enough option that works here 😄

perform empty attachment at the setup phase, and then modify this attachment in-place. Is it reliable enough?

Not sure what exactly do you mean. We support attaching strings or blobs, which are immutable, so you would only be able to rewrite the value somewhere in test runner internals and I'd advise against that.

have some synchronous way to pass data to the reporter,

Again, not sure how the synchronous aspect helps here, unfortunately.

[vitalets]

This is an interesting idea! Having a separate timeout would resolve concerns about running out of the time budget. I'll take a stab at implementing this.

Looking forward for it!

perform empty attachment at the setup phase, and then modify this attachment in-place. Is it reliable enough?

Not sure what exactly do you mean. We support attaching strings or blobs, which are immutable, so you would only be able to rewrite the value somewhere in test runner internals and I'd advise against that.

I mean searching for attachment in testInfo.attachments and modifying it's content. E.g.:

function updateAttachment(name, body) {
  const attachment = testInfo.attachments.find(a => a.name === name);
  attachment.body = body;
}

I can call updateAttachment() after each step during test execution, and then there is no need to finally call testInfo.attach() in fixture teardown. But this is relevant only for my use-case and does not solve this timeout issue in general. And I've checked, modifying attachments does not work 😁

[dgozman]

@vitalets We have discussed this issue with the team, and think we should push back. We recommend explaining users that have fixtures timing out in teardown that it's not considered a normal situation and they should instead fix the fixtures. It is ok for the test to time out, and for a fixture setup to timeout or fail, but cleanup should happen reliably.

I'll see if we can update documentation in this area. Let me know whether we can do something else here.

[vitalets]

Ok, understood. I suggest the following improvements in the docs:

1. clarify how fixture timeout relates to the test timeout. E.g. if fixture timeout exceeds test timeout -> test timeout is used
2. provide some pattern how to reliably teardown the fixture, maybe like this:

import timers from 'timers/promises';

export const test = base.extend({
  myFixture: async ({}, use) => {
    await setup();
    await use();
    await Promise.race([
       teardown(),
       timers.setTimeout(500).then(() => { throw new Error('myFixture teardown timeout'); }),
    ]);
  },
});