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.

Sign up for GitHub

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

Jump to bottom

fix(playwright): test unexpected result if failed and skipped #30276

Closed
wants to merge 5 commits into from
Closed

fix(playwright): test unexpected result if failed and skipped #30276

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
49b2930
fix(playwright): test unexpected result if failed and skipped
BeeMargarida Apr 6, 2024
3436a86
fix(playwright): add check for unexpected skips in failure state
BeeMargarida Apr 6, 2024
7eccbf7
chore(playwright): fix typo in comments
BeeMargarida Apr 8, 2024
c19ebab
fix(playwright): do not check for skipped tests
BeeMargarida Apr 18, 2024
ad586d2
fix(playwright): use expected status
BeeMargarida Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/playwright/src/common/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const passed = results.filter(result => result.status === 'passed');
const passed = results.filter(result => result.status === this.expectedStatus);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably need a test for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 👍

return 'unexpected';
return 'flaky'; // mixed bag
}

Expand Down
34 changes: 34 additions & 0 deletions tests/playwright-test/test-serial.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,40 @@ test('test.describe.serial should work with retry', async ({ runInlineTest }) =>
]);
});

test('test.describe.serial should work with retry when it fails in different places', async ({ runInlineTest }) => {
const result = await runInlineTest({
'a.test.ts': `
import { test, expect } from '@playwright/test';
test.describe.serial('serial suite', () => {
test('test1', async ({}, testInfo) => {
console.log('\\n%%test1');
expect(testInfo.retry).toEqual(0)
});
test('test2', async ({}, testInfo) => {
console.log('\\n%%test2');
expect(testInfo.retry).toEqual(1)
});
test('test3', async ({}) => {
console.log('\\n%%test3');
});
test('test4', async ({}) => {
console.log('\\n%%test4');
});
});
`,
}, { retries: 1 });
expect(result.exitCode).toBe(1);
expect(result.passed).toBe(0);
expect(result.flaky).toBe(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe in this scenario at least two tests test1 and test2 are flaky, aren't they?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed this in the meeting, we'll consider all the implications and other possibilities and @dgozman will come back to you.

expect(result.failed).toBe(1);
expect(result.skipped).toBe(2);
expect(result.outputLines).toEqual([
'test1',
'test2',
'test1',
]);
});

test('test.describe.serial should work with retry and beforeAll failure', async ({ runInlineTest }) => {
const result = await runInlineTest({
'a.test.ts': `
Expand Down
Loading