[Bug]: tests are not skipped when condition depends on fixtures

[felipebn]

Version

1.42.0 and above

Steps to reproduce

Create a test file with:

• top test.describe
• second test.describe with a beforeEach hook and a test

On the top describe block, add a dynamic skip condition based on a fixture like viewport.

Run the test where the skip condition returns true, so all tests should be skipped and the beforeEach hook also.

import {test} from "@playwright/test";

const skipAlwaysWithFixture = () => test.skip(({viewport}) => true);

const skipAlways = () => test.skip(() => true);

test.describe('my test suite', () => {
    skipAlwaysWithFixture();

    // Commenting the one above, this one works as expected
    //skipAlways();

    test.describe('test scenario', () => {
        test.beforeEach(async ({ page }) => {
            console.log('started before each hook');
            await page.goto('https://somerandompage');
        });
        test('test step', async ({ page }) => {
            await page.click('text=Click me');
        });
    });
});

Expected behavior

As of until 1.41.1, it was expected that the beforeEach hook was not invoked if a parent describe contains a dyanmic skip that resolves to true, this is not the case on 1.42.0 and above.

Actual behavior

beforeEach hook is invoked.

This causes tests that should have been skipped to run the beforeEach hook that can lead tests to fail.

Additional context

From trying to debug it, seems like it is related with test.skip using fixtures or not (see example code).

Environment

System:
    OS: macOS 14.4.1
    CPU: (12) arm64 Apple M3 Pro
    Memory: 54.77 MB / 36.00 GB
  Binaries:
    Node: 20.10.0 - ~/.nvm/versions/node/v20.10.0/bin/node
    Yarn: 1.22.22 - ~/.nvm/versions/node/v20.10.0/bin/yarn
    npm: 10.2.3 - ~/.nvm/versions/node/v20.10.0/bin/npm
    pnpm: 9.1.3 - ~/.nvm/versions/node/v20.10.0/bin/pnpm
  Languages:
    Bash: 3.2.57 - /bin/bash
  npmPackages:
    @playwright/test: 1.45.0 => 1.45.0

[pavelfeldman]

Reduced test case:

import { test } from '@playwright/test';

test.describe(() => {
  test.skip(({ viewport }) => true);
  test.beforeEach(() => console.log('before each outer'));

  test.describe(() => {
    test.beforeEach(() => console.log('before each inner'));
    test('test', () => console.log('test'));
  });
});

[felipebn]

I see the PR is merged, thanks for the quick turn around!

qq: what is the ETA for a patch release?