Skip to content

Commit

Permalink
cherry-pick(#22126): fix(tracing): avoid clashing network file names
Browse files Browse the repository at this point in the history
With two contexts in the same test, we can get:
- `<testId>.network` and `<testId>-1.network` files;
- for export, we can copy `<testId>.network` into `<testId>-1.network`
and try to copy into a file when another trace is reading from it.

Fixes #22089.
  • Loading branch information
dgozman committed Apr 1, 2023
1 parent d59972a commit b139ffc
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 3 deletions.
8 changes: 6 additions & 2 deletions packages/playwright-core/src/server/trace/recorder/tracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -261,8 +261,12 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
return {};

// Network file survives across chunks, make a snapshot before returning the resulting entries.
const suffix = state.chunkOrdinal ? `-${state.chunkOrdinal}` : ``;
const networkFile = path.join(state.tracesDir, state.traceName + `${suffix}.network`);
// We should pick a name starting with "traceName" and ending with .network.
// Something like <traceName>someSuffixHere.network.
// However, this name must not clash with any other "traceName".network in the same tracesDir.
// We can use <traceName>-<guid>.network, but "-pwnetcopy-0" suffix is more readable
// and makes it easier to debug future issues.
const networkFile = path.join(state.tracesDir, state.traceName + `-pwnetcopy-${state.chunkOrdinal}.network`);
await fs.promises.copyFile(state.networkFile, networkFile);

const entries: NameValue[] = [];
Expand Down
36 changes: 36 additions & 0 deletions tests/playwright-test/playwright.trace.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,42 @@ test('should not throw with trace: on-first-retry and two retries in the same wo
expect(result.flaky).toBe(6);
});

test('should not mixup network files between contexts', async ({ runInlineTest, server }, testInfo) => {
// NOTE: this test reproduces the issue 10% of the time. Running with --repeat-each=20 helps.
test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/22089' });

const result = await runInlineTest({
'playwright.config.ts': `
export default { use: { trace: 'on' } };
`,
'a.spec.ts': `
import { test, expect } from '@playwright/test';
let page1, page2;
test.beforeAll(async ({ browser }) => {
page1 = await browser.newPage();
await page1.goto("${server.EMPTY_PAGE}");
page2 = await browser.newPage();
await page2.goto("${server.EMPTY_PAGE}");
});
test.afterAll(async () => {
await page1.close();
await page2.close();
});
test('example', async ({ page }) => {
await page.goto("${server.EMPTY_PAGE}");
});
`,
}, { workers: 1, timeout: 15000 });
expect(result.exitCode).toEqual(0);
expect(result.passed).toBe(1);
expect(fs.existsSync(testInfo.outputPath('test-results', 'a-example', 'trace.zip'))).toBe(true);
});

test('should save sources when requested', async ({ runInlineTest }, testInfo) => {
const result = await runInlineTest({
'playwright.config.ts': `
Expand Down
2 changes: 1 addition & 1 deletion tests/playwright-test/ui-mode-test-run.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ test('should stop', async ({ runUITest }) => {
`,
});

await expect(page.getByTitle('Run all')).toBeEnabled();
await expect(page.getByTitle('Run all')).toBeEnabled({ timeout: 15000 });
await expect(page.getByTitle('Stop')).toBeDisabled();

await page.getByTitle('Run all').click();
Expand Down

0 comments on commit b139ffc

Please sign in to comment.