fix(playwright): test unexpected result if failed and skipped #30276
Conversation
Checks if a test results are only failed and skipped. If so, it sets the result as unexpected instead of flaky, since they never passed. Fixes microsoft#28322
|
@microsoft-github-policy-service agree |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| }, { retries: 1 }); | ||
| expect(result.exitCode).toBe(1); | ||
| expect(result.passed).toBe(0); | ||
| expect(result.flaky).toBe(1); |
There was a problem hiding this comment.
I believe in this scenario at least two tests test1 and test2 are flaky, aren't they?
There was a problem hiding this comment.
My apologies, read the bug again, this is an intentional change. I'll discuss in the team meeting if this is change that we want.
There was a problem hiding this comment.
Discussed this in the meeting, we'll consider all the implications and other possibilities and @dgozman will come back to you.
| @@ -290,10 +290,14 @@ export class TestCase extends Base implements reporterTypes.TestCase { | |||
| return 'skipped'; | |||
|
|
|||
| const failures = results.filter(result => result.status !== 'skipped' && result.status !== 'interrupted' && result.status !== this.expectedStatus); | |||
| const skipped = results.filter(result => result.status === 'skipped' && result.status !== this.expectedStatus); | |||
| const passed = results.filter(result => result.status === 'passed'); | |||
There was a problem hiding this comment.
| const passed = results.filter(result => result.status === 'passed'); | |
| const passed = results.filter(result => result.status === this.expectedStatus); |
There was a problem hiding this comment.
We probably need a test for this.
There was a problem hiding this comment.
Not sure I understood, do you mean another test where the expected result is failure?
| if (!failures.length) // all passed | ||
| return 'expected'; | ||
| if (failures.length === results.length) // all failed | ||
| return 'unexpected'; | ||
| if (failures.length && skipped.length && !passed.length) // some failed, none succeeded and the rest were skipped |
There was a problem hiding this comment.
I believe what we need here is instead of lines 297-300 something like:
if (failures.length && !passed.length)
return 'unexpected';as we don't care if there were any "skips".
Test results for "tests 1"2 flaky27371 passed, 671 skipped Merge workflow run. |
|
@BeeMargarida Thank you for the PR! I've researched our past changes in this area, and it turns out things are a bit more complicated:
Given the above, I have decided that it would not be worth it to iterate on this topic together, so I have created a PR myself: #30529, and will most likely close this one. Thank you for sending the PR though! If you are still willing to contribute to Playwright, I'd recommend to pick an issue marked as "open-to-a-pull-request" and start there. |
Hey, thank you for looking into this! 🙌 I completely understand, it seems like a big change in the result logic that could have side effects, so it's okay to improve on this in another PR. As long as the bug is solved, it's a happy ending for me 😄 I'll be on the lookout for the next release! I'll take a look at the issues, always happy to contribute! |
Fixes #28322
Changes:
A test result should be unexpected if it fails (and is not expected) and skips the rest of the runs.
This MR implements this logic, so that runs in serial mode in specific cases (see added unit test) don't cause false positives.