fix(test-runner): property handle artifacts in context of preserveOutput (#7181)

docs/src/test-configuration.md

@@ -21,12 +21,12 @@ You can specify any options either locally in a test file, or globally in the co
   - `'off'` - Do not record trace.
   - `'on'` - Record trace for each test.
   - `'retain-on-failure'` - Record trace for each test, but remove it from successful test runs.
-  - `'retry-with-trace'` - Record trace only when retrying a test.
+  - `'on-first-retry'` - Record trace only when retrying a test for the first time.
 - `video` option - whether to record video for each test, off by default. Video will appear in the test output directory, typically `test-results`.
   - `'off'` - Do not record video.
   - `'on'` - Record video for each test.
   - `'retain-on-failure'` - Record video for each test, but remove all videos from successful test runs.
-  - `'retry-with-video'` - Record video only when retrying a test.
+  - `'on-first-retry'` - Record video only when retrying a test for the first time.
 
 
 ### Global configuration
@@ -47,7 +47,7 @@ module.exports = {
 
     // Artifacts
     screenshot: 'only-on-failure',
-    video: 'retry-with-video',
+    video: 'on-first-retry',
   },
 };
 ```
@@ -68,7 +68,7 @@ const config: PlaywrightTestConfig = {
 
     // Artifacts
     screenshot: 'only-on-failure',
-    video: 'retry-with-video',
+    video: 'on-first-retry',
   },
 };
 export default config;

src/test/cli.ts

@@ -28,7 +28,7 @@ const builtinReporters = ['list', 'line', 'dot', 'json', 'junit', 'null'];
 const tsConfig = 'playwright.config.ts';
 const jsConfig = 'playwright.config.js';
 const defaultConfig: Config = {
-  preserveOutput: 'failures-only',
+  preserveOutput: 'always',
   reporter: [ [defaultReporter] ],
   reportSlowTests: { max: 5, threshold: 15000 },
   timeout: defaultTimeout,

src/test/index.ts

@@ -80,12 +80,18 @@ export const test = _baseTest.extend<PlaywrightTestArgs & PlaywrightTestOptions,
     if (process.env.PWDEBUG)
       testInfo.setTimeout(0);
 
-    const videoMode = typeof video === 'string' ? video : video.mode;
+    let videoMode = typeof video === 'string' ? video : video.mode;
+    if (videoMode === 'retry-with-video')
+      videoMode = 'on-first-retry';
+    if (trace === 'retry-with-trace')
+      trace = 'on-first-retry';
+
+    const captureVideo = (videoMode === 'on' || videoMode === 'retain-on-failure' || (videoMode === 'on-first-retry' && testInfo.retry === 1));
+    const captureTrace = (trace === 'on' || trace === 'retain-on-failure' || (trace === 'on-first-retry' && testInfo.retry === 1));
+
     let recordVideoDir: string | null = null;
     const recordVideoSize = typeof video === 'string' ? undefined : video.size;
-    if (videoMode === 'on' || (videoMode === 'retry-with-video' && !!testInfo.retry))
-      recordVideoDir = testInfo.outputPath('');
-    if (videoMode === 'retain-on-failure') {
+    if (captureVideo) {
       await fs.promises.mkdir(artifactsFolder, { recursive: true });
       recordVideoDir = artifactsFolder;
     }
@@ -137,8 +143,7 @@ export const test = _baseTest.extend<PlaywrightTestArgs & PlaywrightTestOptions,
     const allPages: Page[] = [];
     context.on('page', page => allPages.push(page));
 
-    const collectingTrace = trace === 'on' || trace === 'retain-on-failure' || (trace === 'retry-with-trace' && testInfo.retry);
-    if (collectingTrace) {
+    if (captureTrace) {
       const name = path.relative(testInfo.project.outputDir, testInfo.outputDir).replace(/[\/\\]/g, '-');
       await context.tracing.start({ name, screenshots: true, snapshots: true });
     }
@@ -147,15 +152,16 @@ export const test = _baseTest.extend<PlaywrightTestArgs & PlaywrightTestOptions,
 
     const testFailed = testInfo.status !== testInfo.expectedStatus;
 
-    const saveTrace = trace === 'on' || (testFailed && trace === 'retain-on-failure') || (trace === 'retry-with-trace' && testInfo.retry);
-    if (saveTrace) {
+    const preserveTrace = captureTrace && (trace === 'on' || (testFailed && trace === 'retain-on-failure') || (trace === 'on-first-retry' && testInfo.retry === 1));
+    if (preserveTrace) {
       const tracePath = testInfo.outputPath(`trace.zip`);
       await context.tracing.stop({ path: tracePath });
-    } else if (collectingTrace) {
+    } else if (captureTrace) {
       await context.tracing.stop();
     }
 
-    if (screenshot === 'on' || (screenshot === 'only-on-failure' && testFailed)) {
+    const captureScreenshots = (screenshot === 'on' || (screenshot === 'only-on-failure' && testFailed));
+    if (captureScreenshots) {
       await Promise.all(allPages.map((page, index) => {
         const screenshotPath = testInfo.outputPath(`test-${testFailed ? 'failed' : 'finished'}-${++index}.png`);
         return page.screenshot({ timeout: 5000, path: screenshotPath }).catch(e => {});
@@ -163,7 +169,8 @@ export const test = _baseTest.extend<PlaywrightTestArgs & PlaywrightTestOptions,
     }
     await context.close();
 
-    if (videoMode === 'retain-on-failure' && testFailed) {
+    const preserveVideo = captureVideo && (videoMode === 'on' || (testFailed && videoMode === 'retain-on-failure') || (videoMode === 'on-first-retry' && testInfo.retry === 1));
+    if (preserveVideo) {
       await Promise.all(allPages.map(async page => {
         const v = page.video();
         if (!v)

tests/config/android.config.ts

@@ -30,6 +30,7 @@ const config: Config<CommonOptions & PlaywrightOptions> = {
   globalTimeout: 7200000,
   workers: 1,
   forbidOnly: !!process.env.CI,
+  preserveOutput: process.env.CI ? 'failures-only' : 'always',
   retries: process.env.CI ? 1 : 0,
   reporter: process.env.CI ? [
     [ 'dot' ],

tests/config/default.config.ts

@@ -52,6 +52,7 @@ const config: Config<CommonOptions & PlaywrightOptions> = {
   globalTimeout: 5400000,
   workers: process.env.CI ? 1 : undefined,
   forbidOnly: !!process.env.CI,
+  preserveOutput: process.env.CI ? 'failures-only' : 'always',
   retries: process.env.CI ? 3 : 0,
   reporter: process.env.CI ? [
     [ 'dot' ],

tests/config/electron.config.ts

@@ -30,6 +30,7 @@ const config: Config<CommonOptions & PlaywrightOptions> = {
   globalTimeout: 5400000,
   workers: process.env.CI ? 1 : undefined,
   forbidOnly: !!process.env.CI,
+  preserveOutput: process.env.CI ? 'failures-only' : 'always',
   retries: process.env.CI ? 3 : 0,
   reporter: process.env.CI ? [
     [ 'dot' ],

tests/playwright-test/playwright-test.config.ts

@@ -21,6 +21,7 @@ const config: Config = {
   testIgnore: 'assets/**',
   timeout: 30000,
   forbidOnly: !!process.env.CI,
+  preserveOutput: process.env.CI ? 'failures-only' : 'always',
   projects: [
     { name: 'playwright-test' },
   ]

tests/playwright-test/playwright.spec.ts

@@ -181,10 +181,10 @@ test('should work with video: retain-on-failure', async ({ runInlineTest }, test
   expect(videoFail).toBeTruthy();
 });
 
-test('should work with video: retry-with-video', async ({ runInlineTest }, testInfo) => {
+test('should work with video: on-first-retry', async ({ runInlineTest }, testInfo) => {
   const result = await runInlineTest({
     'playwright.config.ts': `
-      module.exports = { use: { video: 'retry-with-video' }, retries: 1 };
+      module.exports = { use: { video: 'on-first-retry' }, retries: 1 };
     `,
     'a.test.ts': `
       const { test } = pwt;

tests/playwright-test/test-output-dir.spec.ts

@@ -18,19 +18,25 @@ import * as fs from 'fs';
 import * as path from 'path';
 import { test, expect } from './playwright-test-fixtures';
 
-test('should work and remove non-failures on CI', async ({ runInlineTest }, testInfo) => {
+test('should work and remove non-failures', async ({ runInlineTest }, testInfo) => {
   const result = await runInlineTest({
+    'playwright.config.ts': `
+      module.exports = {
+        preserveOutput: 'failures-only',
+        testDir: 'dir',
+      };
+    `,
     'dir/my-test.spec.js': `
       const { test } = pwt;
       test('test 1', async ({}, testInfo) => {
         if (testInfo.retry) {
-          expect(testInfo.outputDir).toContain('dir-my-test-test-1-retry' + testInfo.retry);
-          expect(testInfo.outputPath('foo', 'bar')).toContain(require('path').join('dir-my-test-test-1-retry' + testInfo.retry, 'foo', 'bar'));
+          expect(testInfo.outputDir).toContain('my-test-test-1-chromium-retry' + testInfo.retry);
+          expect(testInfo.outputPath('foo', 'bar')).toContain(require('path').join('my-test-test-1-chromium-retry' + testInfo.retry, 'foo', 'bar'));
           require('fs').writeFileSync(testInfo.outputPath('file.txt'), 'content', 'utf-8');
         } else {
-          expect(testInfo.outputDir).toContain('dir-my-test-test-1');
-          expect(testInfo.outputPath()).toContain('dir-my-test-test-1');
-          expect(testInfo.outputPath('foo', 'bar')).toContain(require('path').join('dir-my-test-test-1', 'foo', 'bar'));
+          expect(testInfo.outputDir).toContain('my-test-test-1-chromium');
+          expect(testInfo.outputPath()).toContain('my-test-test-1-chromium');
+          expect(testInfo.outputPath('foo', 'bar')).toContain(require('path').join('my-test-test-1-chromium', 'foo', 'bar'));
           require('fs').writeFileSync(testInfo.outputPath('file.txt'), 'content', 'utf-8');
         }
         expect(require('fs').existsSync(testInfo.outputDir)).toBe(true);
@@ -38,7 +44,7 @@ test('should work and remove non-failures on CI', async ({ runInlineTest }, test
           throw new Error('Give me retries');
       });
     `,
-  }, { retries: 2 }, { CI: '1' });
+  }, { retries: 2 });
   expect(result.exitCode).toBe(0);
 
   expect(result.results[0].status).toBe('failed');
@@ -54,10 +60,10 @@ test('should work and remove non-failures on CI', async ({ runInlineTest }, test
   expect(result.results[2].status).toBe('passed');
   expect(result.results[2].retry).toBe(2);
 
-  expect(fs.existsSync(testInfo.outputPath('test-results', 'dir-my-test-test-1'))).toBe(true);
-  expect(fs.existsSync(testInfo.outputPath('test-results', 'dir-my-test-test-1-retry1'))).toBe(true);
+  expect(fs.existsSync(testInfo.outputPath('test-results', 'my-test-test-1-chromium'))).toBe(true);
+  expect(fs.existsSync(testInfo.outputPath('test-results', 'my-test-test-1-chromium-retry1'))).toBe(true);
   // Last retry is successfull, so output dir should be removed.
-  expect(fs.existsSync(testInfo.outputPath('test-results', 'dir-my-test-test-1-retry2'))).toBe(false);
+  expect(fs.existsSync(testInfo.outputPath('test-results', 'my-test-test-1-chromium-retry2'))).toBe(false);
 });
 
 test('should include repeat token', async ({runInlineTest}) => {

types/test.d.ts

@@ -967,9 +967,9 @@ export type PlaywrightWorkerOptions = {
  * - `off`: Do not record video.
  * - `on`: Record video for each test.
  * - `retain-on-failure`: Record video for each test, but remove all videos from successful test runs.
- * - `retry-with-video`: Record video only when retrying a test.
+ * - `on-first-retry`: Record video only when retrying a test for the first time.
  */
-export type VideoMode = 'off' | 'on' | 'retain-on-failure' | 'retry-with-video';
+export type VideoMode = 'off' | 'on' | 'retain-on-failure' | 'on-first-retry' | /** deprecated */ 'retry-with-video';
 
 /**
  * Options available to configure each test.
@@ -998,16 +998,12 @@ export type PlaywrightTestOptions = {
    * - `off`: Do not record trace.
    * - `on`: Record trace for each test.
    * - `retain-on-failure`: Record trace for each test, but remove trace from successful test run.
-   * - `retry-with-trace`: Record trace only when retrying a test.
+   * - `on-first-retry`: Record trace only when retrying a test for the first time.
    */
-  trace: 'off' | 'on' | 'retain-on-failure' | 'retry-with-trace';
+  trace: 'off' | 'on' | 'retain-on-failure' | 'on-first-retry' | /** deprecated */ 'retry-with-trace';
 
   /**
    * Whether to record video for each test, off by default.
-   * - `off`: Do not record video.
-   * - `on`: Record video for each test.
-   * - `retain-on-failure`: Record video for each test, but remove all videos from successful test runs.
-   * - `retry-with-video`: Record video only when retrying a test.
    */
   video: VideoMode | { mode: VideoMode, size: ViewportSize };