fix(trace): do not call tracing.stopChunk() twice (#10054)

packages/playwright-test/src/index.ts

@@ -218,7 +218,10 @@ export const test = _baseTest.extend<TestFixtures, WorkerAndFileFixtures>({
       });
     };
 
+    const startedCollectingArtifacts = Symbol('startedCollectingArtifacts');
+
     const onWillCloseContext = async (context: BrowserContext) => {
+      (context as any)[startedCollectingArtifacts] = true;
       if (captureTrace) {
         // Export trace for now. We'll know whether we have to preserve it
         // after the test finishes.
@@ -278,6 +281,12 @@ export const test = _baseTest.extend<TestFixtures, WorkerAndFileFixtures>({
 
     // 5. Collect artifacts from any non-closed contexts.
     await Promise.all(leftoverContexts.map(async context => {
+      // When we timeout during context.close(), we might end up with context still alive
+      // but artifacts being already collected. In this case, do not collect artifacts
+      // for the second time.
+      if ((context as any)[startedCollectingArtifacts])
+        return;
+
       if (preserveTrace)
         await context.tracing.stopChunk({ path: addTraceAttachment() });
       else if (captureTrace)

tests/playwright-test/playwright.artifacts.spec.ts

@@ -277,79 +277,3 @@ test('should work with trace: on-first-retry', async ({ runInlineTest }, testInf
     'report.json',
   ]);
 });
-
-test('should stop tracing with trace: on-first-retry, when not retrying', async ({ runInlineTest }, testInfo) => {
-  const result = await runInlineTest({
-    'playwright.config.ts': `
-      module.exports = { use: { trace: 'on-first-retry' } };
-    `,
-    'a.spec.ts': `
-      const { test } = pwt;
-
-      test.describe('shared', () => {
-        let page;
-        test.beforeAll(async ({ browser }) => {
-          page = await browser.newPage();
-        });
-
-        test.afterAll(async () => {
-          await page.close();
-        });
-
-        test('flaky', async ({}, testInfo) => {
-          expect(testInfo.retry).toBe(1);
-        });
-
-        test('no tracing', async ({}, testInfo) => {
-          const e = await page.context().tracing.stop({ path: 'ignored' }).catch(e => e);
-          expect(e.message).toContain('Must start tracing before stopping');
-        });
-      });
-    `,
-  }, { workers: 1, retries: 1 });
-
-  expect(result.exitCode).toBe(0);
-  expect(result.passed).toBe(1);
-  expect(result.flaky).toBe(1);
-  expect(listFiles(testInfo.outputPath('test-results'))).toEqual([
-    'a-shared-flaky-retry1',
-    '  trace.zip',
-    'report.json',
-  ]);
-});
-
-test('should not throw with trace: on-first-retry and two retries in the same worker', async ({ runInlineTest }, testInfo) => {
-  const files = {};
-  for (let i = 0; i < 6; i++) {
-    files[`a${i}.spec.ts`] = `
-      import { test } from './helper';
-      test('flaky', async ({ myContext }, testInfo) => {
-        await new Promise(f => setTimeout(f, 200 + Math.round(Math.random() * 1000)));
-        expect(testInfo.retry).toBe(1);
-      });
-      test('passing', async ({ myContext }, testInfo) => {
-        await new Promise(f => setTimeout(f, 200 + Math.round(Math.random() * 1000)));
-      });
-    `;
-  }
-  const result = await runInlineTest({
-    ...files,
-    'playwright.config.ts': `
-      module.exports = { use: { trace: 'on-first-retry' } };
-    `,
-    'helper.ts': `
-      const { test: base } = pwt;
-      export const test = base.extend({
-        myContext: [async ({ browser }, use) => {
-          const c = await browser.newContext();
-          await use(c);
-          await c.close();
-        }, { scope: 'worker' }]
-      })
-    `,
-  }, { workers: 3, retries: 1 });
-
-  expect(result.exitCode).toBe(0);
-  expect(result.passed).toBe(6);
-  expect(result.flaky).toBe(6);
-});

tests/playwright-test/playwright.trace.spec.ts

@@ -0,0 +1,112 @@
+/**
+ * Copyright (c) Microsoft Corporation.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { test, expect, stripAscii } from './playwright-test-fixtures';
+import fs from 'fs';
+
+test('should stop tracing with trace: on-first-retry, when not retrying', async ({ runInlineTest }, testInfo) => {
+  const result = await runInlineTest({
+    'playwright.config.ts': `
+      module.exports = { use: { trace: 'on-first-retry' } };
+    `,
+    'a.spec.ts': `
+      const { test } = pwt;
+
+      test.describe('shared', () => {
+        let page;
+        test.beforeAll(async ({ browser }) => {
+          page = await browser.newPage();
+        });
+
+        test.afterAll(async () => {
+          await page.close();
+        });
+
+        test('flaky', async ({}, testInfo) => {
+          expect(testInfo.retry).toBe(1);
+        });
+
+        test('no tracing', async ({}, testInfo) => {
+          const e = await page.context().tracing.stop({ path: 'ignored' }).catch(e => e);
+          expect(e.message).toContain('Must start tracing before stopping');
+        });
+      });
+    `,
+  }, { workers: 1, retries: 1 });
+
+  expect(result.exitCode).toBe(0);
+  expect(result.passed).toBe(1);
+  expect(result.flaky).toBe(1);
+  expect(fs.existsSync(testInfo.outputPath('test-results', 'a-shared-flaky-retry1', 'trace.zip'))).toBeTruthy();
+});
+
+test('should not throw with trace: on-first-retry and two retries in the same worker', async ({ runInlineTest }, testInfo) => {
+  const files = {};
+  for (let i = 0; i < 6; i++) {
+    files[`a${i}.spec.ts`] = `
+      import { test } from './helper';
+      test('flaky', async ({ myContext }, testInfo) => {
+        await new Promise(f => setTimeout(f, 200 + Math.round(Math.random() * 1000)));
+        expect(testInfo.retry).toBe(1);
+      });
+      test('passing', async ({ myContext }, testInfo) => {
+        await new Promise(f => setTimeout(f, 200 + Math.round(Math.random() * 1000)));
+      });
+    `;
+  }
+  const result = await runInlineTest({
+    ...files,
+    'playwright.config.ts': `
+      module.exports = { use: { trace: 'on-first-retry' } };
+    `,
+    'helper.ts': `
+      const { test: base } = pwt;
+      export const test = base.extend({
+        myContext: [async ({ browser }, use) => {
+          const c = await browser.newContext();
+          await use(c);
+          await c.close();
+        }, { scope: 'worker' }]
+      })
+    `,
+  }, { workers: 3, retries: 1 });
+
+  expect(result.exitCode).toBe(0);
+  expect(result.passed).toBe(6);
+  expect(result.flaky).toBe(6);
+});
+
+test('should not throw with trace and timeouts', async ({ runInlineTest }, testInfo) => {
+  const result = await runInlineTest({
+    'playwright.config.ts': `
+      module.exports = { timeout: 2000, repeatEach: 20, use: { trace: 'on' } };
+    `,
+    'a.spec.ts': `
+      const { test } = pwt;
+      test('launches browser', async ({ page }) => {
+      });
+      test('sometimes times out', async ({ page }) => {
+        test.setTimeout(1000);
+        await page.setContent('<div>Hello</div>');
+        await new Promise(f => setTimeout(f, 800 + Math.round(Math.random() * 200)));
+      });
+    `,
+  }, { workers: 2 });
+
+  expect(result.exitCode).toBe(1);
+  expect(stripAscii(result.output)).not.toContain('tracing.stopChunk:');
+  expect(stripAscii(result.output)).not.toContain('tracing.stop:');
+});