[BUG] Ambiguous test outcome and status for serial mode

[agoldis]

System info

• Playwright Version: v1.39.0, v1.40.0
• Operating System: All
• Browser: n/a
• Other info:

Source code

• I provided exact source code that allows reproducing the issue locally.

Link to the GitHub repository with the repro

https://github.com/currents-dev/pw-issue-2023-11-23-001

Steps

• Checkout the demo repo, follow README instructions
• Run the test for 1.39
• Run the test for 1.40

Expected

The following test suite:

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

test.describe.configure({ mode: "serial", retries: 1 });

test("A", async ({ page }, { retry }) => {
  if (retry === 0) {
    throw new Error("oh!");
  }
  if (retry === 1) {
    expect(true).toBe(true);
  }

});

test("B", async ({ page }, { retry }) => {
  throw new Error("oh!");
});

Is supposed to produce the following outcome:

• Test A results status: failed, passed

• Test B results status: skipped, failed

• onTestBegin should be invoked for every attempt of every test in a serial group as in 1.39

• all tests should exhaust all attempts in a serial group

Actual

1.40 runs serial tests group differently and produces different results:

• Test A results status: failed, passed

• Test B results status: failed

• 1.40 doesn't invoke onTestBegin for test B when A fails

• 1.40 doesn't retry test B and "swallows" the last retry

[mxschmitt]

Bisect NPM:
1.40.0-alpha-oct-25-2023 bad
1.40.0-alpha-oct-24-2023 good

RANGE: fe7847b...0bb9f7c

Looks caused by #27762

[dgozman]

@agoldis This is an intended change. After A fails, B is not run at all, so we do not produce a "skipped" run. Would you like to still see a skipped run for B? What's the reason? Please provide as much details about your usecase as you can.

[agoldis]

Would you like to still see a skipped run for B?

• Yes, if that's possible, I understand there may be many reasons not to keep the original behaviour, but I believe 1.39 was handling serial groups more correctly, at least from a reporter author perspective.

What's the reason?

• It's inconsistent with 1.39 and produces different results
• Test B only runs once and never retries, although we set retries: 1
• It doesn't match the documentation
  • https://playwright.dev/docs/test-parallel#serial-mode

    • ... if one of the serial tests fails, all subsequent tests are skipped.

  • https://playwright.dev/docs/test-retries#serial-mode

    • ...If one of the tests fails, all subsequent tests are skipped

  • I read skipped as getting status skipped, while 1.40 simply doesn't run the test. But maybe I am mistaken, please correct me.
• It is impossible (to my best knowledge) for reporters to interpret what actually happened - 1.40 produces one attempt for test B with retry index 0. A reporter would register the following results in onTestEnd: A - 0: failed, 1: passed, B - 0: failed so it may seem as if both A and B ran one after another as part of the same serial "attempt", which isn't supposed to happen. In other words, reporters can't "register" the fact that some tests in a serial group were skipped because of a preceding test failure. 1.39 was triggering both onTestBegin and onTestEnd for skipped serial tests and that was working great!

I hope it makes sense... We just need a clear way to identify what happened when interpreting results in a reporter.

P.S.

Looking at the fix at #27762 and the original issue at #27455, I think the real issue is how PW detects flaky tests.

That's why I also opened #28322. I understand it is a disruptive proposal, but let me try.

I think a test should be marked as flaky if and only if:

• expectedStatus is not skipped and
• non-skipped and non-interrupted attempts have 1 status that matches the expectedStatus and one that doesn't

For example, 1.39 marks the following test with 'expectedStatus: passed' and 4 attempts: skipped, failed, skipped, failed as flaky, however, all the executed attempts produce similar results: failed, so I would not consider this test as flaky.

Sorry for my ignorance, may be I am missing something - I am new here.

[dgozman]

We decided to revert #27762 for now. However, the definition of a flaky test must differentiate between tests skipped with test.skip() vs tests skipped because of serial mode failure or any other similar scenario. Otherwise, it does not work in some of the cases. We'll think more about it.

[agoldis]

Thanks for the quick turnaround @dgozman!
Exposing something like a skipped cause or a new status would do the job. I would be happy to contribute if you provide a direction.