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

[BUG] - Skipped tests marks as failed #23201

Open
Nck1969 opened this issue May 22, 2023 · 8 comments
Open

[BUG] - Skipped tests marks as failed #23201

Nck1969 opened this issue May 22, 2023 · 8 comments
Assignees
Labels
feature-test-runner Playwright test specific issues

Comments

@Nck1969
Copy link

Nck1969 commented May 22, 2023

System info

  • Playwright Version: [v1.33.0, v1.34.0]
  • Operating System: [All, Windows 11, Ubuntu 20, macOS 13.2, etc.]
  • Browser: [All, Chromium, Firefox, WebKit]
  • Other info:

Source code

test.describe.serial("Tests", () => {
  const random = Math.random();

  test("Test can be skipped", () => {
    test.skip(random > 0.5, "Some skipped test");
  });

  test("Flaky test", () => {
    expect(random).toBeGreaterThan(0.5); // some flaky step  });
  });

  test("Passed test", () => expect(3).toBe(3));
});

Description:
Skipped test marks as failed if:

  1. All tests in describe.serial block;
  2. "retries: 2" - settled in tests config file;
  3. "Test can be skipped": first retry - pass, second - skipped;
  4. "Flaky test": first retry - failed, second - passed.

Expected

"Test can be skipped" - marks as passed and tests running exit with 0.

Actual

"Test can be skipped" - marks as failed and tests running exit with 1.

image image image image image
@dgozman dgozman added the v1.35 label May 23, 2023
yury-s added a commit to yury-s/playwright that referenced this issue Jun 3, 2023
Tests may be marked as skipped or failed dyanamically, the final outcome should take that
into account. This will also be useful in merge report where same test can come from
different platforms with different expectations.

Fixes microsoft#23201
yury-s added a commit to yury-s/playwright that referenced this issue Jun 3, 2023
Tests may be marked as skipped or failed dyanamically, the final outcome should take that
into account. This will also be useful in merge report where same test can come from
different platforms with different expectations.

Fixes microsoft#23201
@yury-s
Copy link
Member

yury-s commented Jun 5, 2023

@Nck1969 what is the use case where you want to skip the test on one run and the pass on a retry? We are debating whether we should throw if the expectation changes on retry to avoid this sort of ambiguity.

@Nck1969
Copy link
Author

Nck1969 commented Jun 6, 2023

@Nck1969 what is the use case where you want to skip the test on one run and the pass on a retry? We are debating whether we should throw if the expectation changes on retry to avoid this sort of ambiguity.

I don't want to 'skip the test on one run and the pass on a retry' I just add skip condition to the test cause some element on page can be or not to be on the page.
And also have retries in my config file cause now I need it, but wont remove it in feature.
In this case I expected that process return 0 and result marks as passed cause on the 3'rd iteration all tests are passed.

@aslushnikov aslushnikov added v1.36 and removed v1.35 labels Jun 6, 2023
@yury-s
Copy link
Member

yury-s commented Jun 6, 2023

I don't want to 'skip the test on one run and the pass on a retry' I just add skip condition to the test cause some element on page can be or not to be on the page.
And also have retries in my config file cause now I need it, but wont remove it in feature.

Can the presence of the element change on retry? As long as you consistently mark the test as skipped on the first run and subsequent retries it will be marked as skipped and will not fail the run.

@Nck1969
Copy link
Author

Nck1969 commented Jun 9, 2023

I don't want to 'skip the test on one run and the pass on a retry' I just add skip condition to the test cause some element on page can be or not to be on the page.
And also have retries in my config file cause now I need it, but wont remove it in feature.

Can the presence of the element change on retry? As long as you consistently mark the test as skipped on the first run and subsequent retries it will be marked as skipped and will not fail the run.

Yes, sometimes the presence of the element can change on retry

@aslushnikov aslushnikov added feature-test-runner Playwright test specific issues and removed v1.36 labels Jul 7, 2023
@sky-brunoteixeira
Copy link

sky-brunoteixeira commented Oct 10, 2023

image
image

The problem for me is that for tests with a skip condition if for some reason fail on beforeEach and on retry are skipped, at the end the test is still marked as failed. I would expect the skip on retry to be the actual status of the test.

@RyDoRe
Copy link

RyDoRe commented Oct 12, 2023

Have the same problem as well.
I have a setup with 2 retries and in skipped tests for which the beforeEach hook fails, the second execution is done but not marked as anything and the test still get's marked as failed

@Gmartinica
Copy link

Tried in 1.42 and still have the same problem.

I use test.skip(condition) in this structure for test:

test.beforeEach({
test.skip(true)
})

test({
test.skip(true)
})

test.afterEach({
test.skip(true)
})

and test report shows the test as failing instead of skipped with retries : 0
Furthermore, adding retries results in the test being labeled as failed initially, then skipped, ultimately leading to a 'flaky' result. However index.html report shows the test marked as failed

Removing the beforeEach and afterEach hooks.

test({
test.skip(true)
})

Correctly shows the test as skipped.
@aslushnikov @mxschmitt Could we please have any updates on this?

@josephwynn-sc
Copy link

josephwynn-sc commented Apr 23, 2024

We just tested upgrading Playwright from 1.36.2 to 1.43.1 and found this bug is blocking us. We use the following pattern:

test.describe("Some browser-specific test suite", () => {
  test.skip(
    ({ browserName }) => browserName !== "chromium",
    "Tests only work in Chromium",
  );

  test("...", () => {...});
});

In previous versions, everything inside the test.describe was skipped. The test.skip documentation even describes this exact use case:

You can skip all tests in a file or test.describe() group based on some condition with a single test.skip(callback, description) call.

However now it seems like Playwright is ignoring the test.skip call, and these tests are failing in non-Chromium browsers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-test-runner Playwright test specific issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants