fix(test runner): fixture teardown double error, testInfo.attach() (#11365)

- Use file path, not content to calculate the attachment hash.
- Always cleanup fixture from the list on teardown, to avoid reporting
  teardown error multiple times: from the test, and from the cleanup.

packages/playwright-core/src/utils/utils.ts

@@ -371,19 +371,6 @@ export function monotonicTime(): number {
   return seconds * 1000 + (nanoseconds / 1000 | 0) / 1000;
 }
 
-class HashStream extends stream.Writable {
-  private _hash = crypto.createHash('sha1');
-
-  override _write(chunk: Buffer, encoding: string, done: () => void) {
-    this._hash.update(chunk);
-    done();
-  }
-
-  digest(): string {
-    return this._hash.digest('hex');
-  }
-}
-
 export function objectToArray(map?:  { [key: string]: any }): NameValue[] | undefined {
   if (!map)
     return undefined;
@@ -402,17 +389,6 @@ export function arrayToObject(array?: NameValue[]): { [key: string]: string } |
   return result;
 }
 
-export async function calculateFileSha1(filename: string): Promise<string> {
-  const hashStream = new HashStream();
-  const stream = fs.createReadStream(filename);
-  stream.on('open', () => stream.pipe(hashStream));
-  await new Promise((f, r) => {
-    hashStream.on('finish', f);
-    hashStream.on('error', r);
-  });
-  return hashStream.digest();
-}
-
 export function calculateSha1(buffer: Buffer | string): string {
   const hash = crypto.createHash('sha1');
   hash.update(buffer);

packages/playwright-test/src/fixtures.ts

@@ -96,15 +96,18 @@ class Fixture {
   private async _teardownInternal() {
     if (typeof this.registration.fn !== 'function')
       return;
-    for (const fixture of this.usages)
+    try {
-      await fixture.teardown();
+      for (const fixture of this.usages)
-    this.usages.clear();
+        await fixture.teardown();
-    if (this._useFuncFinished) {
+      this.usages.clear();
-      debugTest(`teardown ${this.registration.name}`);
+      if (this._useFuncFinished) {
-      this._useFuncFinished.resolve();
+        debugTest(`teardown ${this.registration.name}`);
-      await this._selfTeardownComplete;
+        this._useFuncFinished.resolve();
+        await this._selfTeardownComplete;
+      }
+    } finally {
+      this.runner.instanceForId.delete(this.registration.id);
     }
-    this.runner.instanceForId.delete(this.registration.id);
   }
 }
 

packages/playwright-test/src/workerRunner.ts

@@ -30,7 +30,7 @@ import { Annotations, TestCaseType, TestError, TestInfo, TestInfoImpl, TestStepI
 import { ProjectImpl } from './project';
 import { FixtureRunner } from './fixtures';
 import { DeadlineRunner, raceAgainstDeadline } from 'playwright-core/lib/utils/async';
-import { calculateFileSha1 } from 'playwright-core/lib/utils/utils';
+import { calculateSha1 } from 'playwright-core/lib/utils/utils';
 
 const removeFolderAsync = util.promisify(rimraf);
 
@@ -268,7 +268,7 @@ export class WorkerRunner extends EventEmitter {
         if ((options.path !== undefined ? 1 : 0) + (options.body !== undefined ? 1 : 0) !== 1)
           throw new Error(`Exactly one of "path" and "body" must be specified`);
         if (options.path) {
-          const hash = await calculateFileSha1(options.path);
+          const hash = calculateSha1(options.path);
           const dest = testInfo.outputPath('attachments', hash + path.extname(options.path));
           await fs.promises.mkdir(path.dirname(dest), { recursive: true });
           await fs.promises.copyFile(options.path, dest);

tests/playwright-test/fixture-errors.spec.ts

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { test, expect } from './playwright-test-fixtures';
+import { test, expect, countTimes, stripAscii } from './playwright-test-fixtures';
 
 test('should handle fixture timeout', async ({ runInlineTest }) => {
   const result = await runInlineTest({
@@ -435,3 +435,23 @@ test('should not teardown when setup times out', async ({ runInlineTest }) => {
   expect(result.output.split('\n').filter(line => line.startsWith('%%'))).toEqual([
   ]);
 });
+
+test('should not report fixture teardown error twice', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.spec.ts': `
+      const test = pwt.test.extend({
+        fixture: async ({ }, use) => {
+          await use();
+          throw new Error('Oh my error');
+        },
+      });
+      test('good', async ({ fixture }) => {
+      });
+    `,
+  }, { reporter: 'list' });
+  expect(result.exitCode).toBe(1);
+  expect(result.failed).toBe(1);
+  expect(result.output).toContain('Error: Oh my error');
+  expect(stripAscii(result.output)).toContain(`throw new Error('Oh my error')`);
+  expect(countTimes(result.output, 'Oh my error')).toBe(2);
+});

tests/playwright-test/playwright-test-fixtures.ts

@@ -247,7 +247,7 @@ export function stripAscii(str: string): string {
   return str.replace(asciiRegex, '');
 }
 
-function countTimes(s: string, sub: string): number {
+export function countTimes(s: string, sub: string): number {
   let result = 0;
   for (let index = 0; index !== -1;) {
     index = s.indexOf(sub, index);

tests/playwright-test/reporter-attachment.spec.ts

@@ -80,7 +80,6 @@ test('render trace attachment', async ({ runInlineTest }) => {
   expect(result.exitCode).toBe(1);
 });
 
-
 test(`testInfo.attach errors`, async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'a.test.js': `
@@ -97,9 +96,50 @@ test(`testInfo.attach errors`, async ({ runInlineTest }) => {
     `,
   }, { reporter: 'line', workers: 1 });
   const text = stripAscii(result.output).replace(/\\/g, '/');
-  expect(text).toMatch(/Error: ENOENT: no such file or directory, open '.*foo.txt.*'/);
+  expect(text).toMatch(/Error: ENOENT: no such file or directory, copyfile '.*foo.txt.*'/);
   expect(text).toContain(`Exactly one of "path" and "body" must be specified`);
   expect(result.passed).toBe(0);
   expect(result.failed).toBe(3);
   expect(result.exitCode).toBe(1);
 });
+
+test(`testInfo.attach error in fixture`, async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.js': `
+      const test = pwt.test.extend({
+        fixture: async ({}, use, testInfo) => {
+          await use();
+          await testInfo.attach('name', { path: 'foo.txt' });
+        },
+      });
+      test('fail1', async ({ fixture }) => {
+      });
+    `,
+  }, { reporter: 'line', workers: 1 });
+  const text = stripAscii(result.output).replace(/\\/g, '/');
+  expect(text).toMatch(/Error: ENOENT: no such file or directory, copyfile '.*foo.txt.*'/);
+  expect(result.exitCode).toBe(1);
+  expect(result.passed).toBe(0);
+  expect(result.failed).toBe(1);
+});
+
+test(`testInfo.attach success in fixture`, async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.js': `
+      const test = pwt.test.extend({
+        fixture: async ({}, use, testInfo) => {
+          const filePath = testInfo.outputPath('foo.txt');
+          require('fs').writeFileSync(filePath, 'hello');
+          await use();
+          await testInfo.attach('name', { path: filePath });
+        },
+      });
+      test('success', async ({ fixture }) => {
+        expect(true).toBe(false);
+      });
+    `,
+  });
+  expect(result.exitCode).toBe(1);
+  expect(result.failed).toBe(1);
+  expect(stripAscii(result.output)).toContain('attachment #1: name (text/plain)');
+});

tests/playwright-test/reporter-raw.spec.ts

@@ -155,7 +155,7 @@ test(`testInfo.attach should save attachments via path`, async ({ runInlineTest
     expect(result.attachments[0].name).toBe('foo');
     expect(result.attachments[0].contentType).toBe('application/json');
     const p = result.attachments[0].path;
-    expect(p).toMatch(/[/\\]attachments[/\\]01a5667d100fac2200bf40cf43083fae0580c58e\.json$/);
+    expect(p).toMatch(/[/\\]attachments[/\\][0-9a-f]+\.json$/);
     const contents = fs.readFileSync(p);
     expect(contents.toString()).toBe('We <3 Playwright!');
   }
@@ -164,7 +164,7 @@ test(`testInfo.attach should save attachments via path`, async ({ runInlineTest
     expect(result.attachments[0].name).toBe('foo');
     expect(result.attachments[0].contentType).toBe('image/png');
     const p = result.attachments[0].path;
-    expect(p).toMatch(/[/\\]attachments[/\\]01a5667d100fac2200bf40cf43083fae0580c58e\.json$/);
+    expect(p).toMatch(/[/\\]attachments[/\\][0-9a-f]+\.json$/);
     const contents = fs.readFileSync(p);
     expect(contents.toString()).toBe('We <3 Playwright!');
   }
@@ -173,7 +173,7 @@ test(`testInfo.attach should save attachments via path`, async ({ runInlineTest
     expect(result.attachments[0].name).toBe('example.png');
     expect(result.attachments[0].contentType).toBe('x-playwright/custom');
     const p = result.attachments[0].path;
-    expect(p).toMatch(/[/\\]attachments[/\\]01a5667d100fac2200bf40cf43083fae0580c58e\.json$/);
+    expect(p).toMatch(/[/\\]attachments[/\\][0-9a-f]+\.json$/);
     const contents = fs.readFileSync(p);
     expect(contents.toString()).toBe('We <3 Playwright!');
   }
@@ -182,7 +182,7 @@ test(`testInfo.attach should save attachments via path`, async ({ runInlineTest
     expect(result.attachments[0].name).toBe('foo');
     expect(result.attachments[0].contentType).toBe('application/octet-stream');
     const p = result.attachments[0].path;
-    expect(p).toMatch(/[/\\]attachments[/\\]01a5667d100fac2200bf40cf43083fae0580c58e\.this-extension-better-not-map-to-an-actual-mimetype$/);
+    expect(p).toMatch(/[/\\]attachments[/\\][0-9a-f]+\.this-extension-better-not-map-to-an-actual-mimetype$/);
     const contents = fs.readFileSync(p);
     expect(contents.toString()).toBe('We <3 Playwright!');
   }