[Regression]: test.beforeEach() still runs when test.skip() returns true when it references fixture param

[mattjennings]

Last Good Version

1.42.1

First Bad Version

1.43.0

Steps to reproduce

1. Clone repo at https://github.com/mattjennings/playwright-fixture-skip-regression
2. Run npm install && npx playwright install which will install @playwright/[email protected]
3. Run npx playwright test
4. Notice the test failures because beforeEach was ran (and threw an error, to demonstrate the problem)
5. Try again with @playwright/[email protected] and see they are all skipped with no errors

Expected behavior

All tests should be skipped and test.beforeEach should not be run when test.skip() references a custom fixture.

Actual behavior

The describe block that uses test.skip({ foo }) still runs the test.beforeEach

Running 6 tests using 5 workers

  -  1 [chromium] › example.spec.ts:35:3 › not using fixture param › should be skipped
  ✘  2 [firefox] › example.spec.ts:21:3 › using fixture param › should be skipped (3ms)
  -  3 [firefox] › example.spec.ts:35:3 › not using fixture param › should be skipped
  ✘  4 [chromium] › example.spec.ts:21:3 › using fixture param › should be skipped (3ms)
  ✘  5 [webkit] › example.spec.ts:21:3 › using fixture param › should be skipped (3ms)
  -  6 [webkit] › example.spec.ts:35:3 › not using fixture param › should be skipped


  1) [chromium] › example.spec.ts:21:3 › using fixture param › should be skipped ───────────────────

    Error: beforeEach should not have run

      16 |
      17 |   test.beforeEach(async ({}) => { 
    > 18 |       throw new Error('beforeEach should not have run');
         |             ^
      19 |   });
      20 |
      21 |   test('should be skipped', () => {

        at /Users/mattjennings/tmp/fixture-skip/tests/example.spec.ts:18:13

  2) [firefox] › example.spec.ts:21:3 › using fixture param › should be skipped ────────────────────

    Error: beforeEach should not have run

      16 |
      17 |   test.beforeEach(async ({}) => { 
    > 18 |       throw new Error('beforeEach should not have run');
         |             ^
      19 |   });
      20 |
      21 |   test('should be skipped', () => {

        at /Users/mattjennings/tmp/fixture-skip/tests/example.spec.ts:18:13

  3) [webkit] › example.spec.ts:21:3 › using fixture param › should be skipped ─────────────────────

    Error: beforeEach should not have run

      16 |
      17 |   test.beforeEach(async ({}) => { 
    > 18 |       throw new Error('beforeEach should not have run');
         |             ^
      19 |   });
      20 |
      21 |   test('should be skipped', () => {

        at /Users/mattjennings/tmp/fixture-skip/tests/example.spec.ts:18:13

  3 failed
    [chromium] › example.spec.ts:21:3 › using fixture param › should be skipped ────────────────────
    [firefox] › example.spec.ts:21:3 › using fixture param › should be skipped ─────────────────────
    [webkit] › example.spec.ts:21:3 › using fixture param › should be skipped ──────────────────────
  3 skipped

Additional context

This is a breaking change in behaviour for me, as I make use of an expensive fixture param in beforeEach({ expensiveFoo }) that I do not want to execute if the test has been skipped.

Previously, beforeAll() ran before test.skip() and beforeEach() ran after, and I haven't seen anything in the release notes to indicate this was an intentional change.

Environment

System:
    OS: macOS 14.5
    CPU: (10) arm64 Apple M1 Max
    Memory: 304.58 MB / 32.00 GB
  Binaries:
    Node: 20.14.0 - ~/.nvm/versions/node/v20.14.0/bin/node
    npm: 10.7.0 - ~/.nvm/versions/node/v20.14.0/bin/npm
    pnpm: 8.12.0 - /opt/homebrew/bin/pnpm
    bun: 1.0.0 - ~/.bun/bin/bun
  IDEs:
    VSCode: 1.90.2 - /usr/local/bin/code
  Languages:
    Bash: 3.2.57 - /bin/bash
  npmPackages:
    @playwright/test: 1.43.0 => 1.43.0

[mattjennings]

There is a further regression here that I've just found. I can make a separate issue for it if desired, but it's pretty similar:

const test = base.extend<{ foo: string, errorFoo: void }>({
  foo: async ({}, use) => {
    await use('foo')
  },
  errorFoo: async ({}, use) => {
    await use()
    throw new Error('foo')
  }
});

test.describe('using fixture param', () => {
  test.skip(({ foo }) => {    
      return true
  });

  test.beforeEach(async ({ errorFoo }) => {   
    errorFoo;  
  });

  test('should be skipped', () => {
      expect(true).toBe(false);
  });
})

In 1.41.2, errorFoo is not executed, but in 1.42.0 it is.

[pavelfeldman]

Folding into #31425