From 13d48da84a0c35f9e24287e75647a8819385925f Mon Sep 17 00:00:00 2001
From: Dmitry Gozman <dgozman@gmail.com>
Date: Wed, 30 Sep 2020 16:52:21 -0700
Subject: [PATCH] test: record trace/videos on retries (#4009)

---
 test/fixtures.ts            | 17 ++++++++++-------
 test/playwright.fixtures.ts | 32 ++++++++++----------------------
 test/trace.spec.ts          |  9 +++++----
 3 files changed, 25 insertions(+), 33 deletions(-)

diff --git a/test/fixtures.ts b/test/fixtures.ts
index 369cabe070..1527694564 100644
--- a/test/fixtures.ts
+++ b/test/fixtures.ts
@@ -20,6 +20,7 @@ import childProcess from 'child_process';
 import fs from 'fs';
 import path from 'path';
 import util from 'util';
+import os from 'os';
 import type { Browser, BrowserContext, BrowserType, Page } from '../index';
 import { Connection } from '../lib/client/connection';
 import { Transport } from '../lib/protocol/transport';
@@ -31,6 +32,7 @@ export { expect } from '@playwright/test/out/matcher.fixtures';
 export { config } from '@playwright/test-runner';
 
 const removeFolderAsync = util.promisify(require('rimraf'));
+const mkdtempAsync = util.promisify(fs.mkdtemp);
 
 type AllParameters = {
   wire: boolean;
@@ -126,19 +128,20 @@ fixtures.overrideWorkerFixture('playwright', async ({ browserName, testWorkerInd
   }
 });
 
-fixtures.defineTestFixture('createUserDataDir', async ({testOutputPath}, runTest) => {
-  let counter = 0;
+fixtures.defineTestFixture('createUserDataDir', async ({}, runTest) => {
   const dirs: string[] = [];
   async function createUserDataDir() {
-    const dir = testOutputPath(`user-data-dir-${counter++}`);
+    // We do not put user data dir in testOutputPath,
+    // because we do not want to upload them as test result artifacts.
+    //
+    // Additionally, it is impossible to upload user data dir after test run:
+    // - Firefox removes lock file later, presumably from another watchdog process?
+    // - WebKit has circular symlinks that makes CI go crazy.
+    const dir = await mkdtempAsync(path.join(os.tmpdir(), 'playwright-test-'));
     dirs.push(dir);
-    await fs.promises.mkdir(dir, { recursive: true });
     return dir;
   }
   await runTest(createUserDataDir);
-  // Remove user data dirs, because we cannot upload them as test result artifacts.
-  // - Firefox removes lock file later, repsumably from another watchdog process?
-  // - WebKit has circular symlinks that makes CI go crazy.
   await Promise.all(dirs.map(dir => removeFolderAsync(dir).catch(e => {})));
 });
 
diff --git a/test/playwright.fixtures.ts b/test/playwright.fixtures.ts
index ca13548fad..aff1f585d2 100644
--- a/test/playwright.fixtures.ts
+++ b/test/playwright.fixtures.ts
@@ -15,16 +15,8 @@
  */
 
 import { config, fixtures as baseFixtures } from '@playwright/test-runner';
-import fs from 'fs';
-import os from 'os';
-import path from 'path';
-import util from 'util';
 import type { Browser, BrowserContext, BrowserContextOptions, BrowserType, LaunchOptions, Page } from '../index';
 
-const mkdtempAsync = util.promisify(fs.mkdtemp);
-const removeFolderAsync = util.promisify(require('rimraf'));
-
-
 // Parameter declarations ------------------------------------------------------
 
 type PlaywrightParameters = {
@@ -86,8 +78,6 @@ type PlaywrightTestFixtures = {
   context: BrowserContext;
   // Page instance for test.
   page: Page;
-  // Temporary directory for this test's artifacts.
-  tmpDir: string;
 };
 
 export const fixtures = baseFixtures
@@ -162,12 +152,16 @@ fixtures.defineWorkerFixture('isLinux', async ({platform}, test) => {
 
 // Test fixtures definitions ---------------------------------------------------
 
-fixtures.defineTestFixture('defaultContextOptions', async ({ testRelativeArtifactsPath, trace }, runTest) => {
-  await runTest({
-    relativeArtifactsPath: testRelativeArtifactsPath,
-    recordTrace: trace,
-    recordVideos: trace,
-  });
+fixtures.defineTestFixture('defaultContextOptions', async ({ testRelativeArtifactsPath, trace, testInfo }, runTest) => {
+  if (trace || testInfo.retry) {
+    await runTest({
+      relativeArtifactsPath: testRelativeArtifactsPath,
+      recordTrace: true,
+      recordVideos: true,
+    });
+  } else {
+    await runTest({});
+  }
 });
 
 fixtures.defineTestFixture('contextFactory', async ({ browser, defaultContextOptions, testInfo, screenshotOnFailure, testOutputPath }, runTest) => {
@@ -203,12 +197,6 @@ fixtures.defineTestFixture('page', async ({context}, runTest) => {
   // Context fixture is taking care of closing the page.
 });
 
-fixtures.defineTestFixture('tmpDir', async ({ }, runTest) => {
-  const tmpDir = await mkdtempAsync(path.join(os.tmpdir(), 'playwright-test-'));
-  await runTest(tmpDir);
-  await removeFolderAsync(tmpDir).catch(e => { });
-});
-
 fixtures.overrideTestFixture('testParametersArtifactsPath', async ({ browserName, platform }, runTest) => {
   await runTest(browserName + '-' + platform);
 });
diff --git a/test/trace.spec.ts b/test/trace.spec.ts
index 37f43b10c5..7eb870fe94 100644
--- a/test/trace.spec.ts
+++ b/test/trace.spec.ts
@@ -19,10 +19,11 @@ import type * as trace from '../types/trace';
 import * as path from 'path';
 import * as fs from 'fs';
 
-it('should record trace', async ({browserType, defaultBrowserOptions, server, tmpDir}) => {
+it('should record trace', async ({browserType, defaultBrowserOptions, server, testOutputPath}) => {
+  const artifactsPath = testOutputPath('trace');
   const browser = await browserType.launch({
     ...defaultBrowserOptions,
-    artifactsPath: tmpDir,
+    artifactsPath,
   });
   const context = await browser.newContext({ recordTrace: true });
   const page = await context.newPage();
@@ -31,7 +32,7 @@ it('should record trace', async ({browserType, defaultBrowserOptions, server, tm
   await context.close();
   await browser.close();
 
-  const traceFile = path.join(tmpDir, 'playwright.trace');
+  const traceFile = path.join(artifactsPath, 'playwright.trace');
   const traceFileContent = await fs.promises.readFile(traceFile, 'utf8');
   const traceEvents = traceFileContent.split('\n').filter(line => !!line).map(line => JSON.parse(line)) as trace.TraceEvent[];
 
@@ -51,7 +52,7 @@ it('should record trace', async ({browserType, defaultBrowserOptions, server, tm
   expect(gotoEvent.value).toBe(url);
 
   expect(gotoEvent.snapshot).toBeTruthy();
-  expect(fs.existsSync(path.join(tmpDir, 'trace-resources', gotoEvent.snapshot!.sha1))).toBe(true);
+  expect(fs.existsSync(path.join(artifactsPath, 'trace-resources', gotoEvent.snapshot!.sha1))).toBe(true);
 });
 
 it('should require artifactsPath', async ({browserType, defaultBrowserOptions}) => {