feat(reporter): report `TestStep#attachments` #34037

Skn0tt · 2024-12-16T17:46:58Z

Adds a TestStep.attachments array to the Reporter API that shows all attachments created by the step. Removes the artificial attach "foo" step for all calls except TestInfo.attach().

This also means some changes to the HTML report attachment links introduced in #33267:

the icon button goes from the removed attach "foo" step to their parent step
the internal anchoring now goes by attachment index instead of the attachment name

docs/src/test-reporter-api/class-teststep.md

packages/playwright/src/index.ts

packages/playwright/src/matchers/expect.ts

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

packages/playwright/src/matchers/toMatchSnapshot.ts

packages/playwright/src/worker/testInfo.ts

Skn0tt · 2024-12-20T12:09:56Z

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

@@ -393,3 +391,48 @@ test('should show custom fixture titles in actions tree', async ({ runUITest })
    /After Hooks[\d.]+m?s/,
  ]);
 });
+
+test('attachments tab shows all but top-level .push attachments', async ({ runUITest }) => {


This test shows a real regression from this change. As intended, test.info().attachments.push will no longer show up as an action. The trace viewer only stores attachments in AfterActionTraceEvent though. So top-level attachments, which live outside of an action, no longer show up in traces.

I don't think we'll want to merge a regression like this, so i'll make an exception for top-level attachment.push calls, and emit them as actions to the trace viewer.

Sounds fine for now. I see that's already done in the testInfo.ts?

We can follow up by adding a timestamp to attachments and emitting a separate "testend" event in the trace with all the leftover attachments. At that point we can possibly remove "attach foo" steps as well. Up for discussion.

Yes, that's already done 👍 Separate event post execution sounds good.

Skn0tt

Thanks for the review, that was super useful! I've addressed the feedback. I also had another look at what this change means for the trace viewer. There was a problem with top-level attachments that would have gone missing from the trace-viewer - added a workaround for that, see comment above.

Please have another look!

packages/playwright/src/runner/dispatcher.ts

dgozman · 2024-12-30T09:18:44Z

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

@@ -566,7 +566,7 @@ test('should opt out of attachments', async ({ runInlineTest, server }, testInfo
    'After Hooks',
  ]);
  expect(trace.actions[1].attachments).toEqual(undefined);
-  expect([...trace.resources.keys()].filter(f => f.startsWith('resources/'))).toHaveLength(0);
+  expect([...trace.resources.keys()].filter(f => f.startsWith('resources/'))).toEqual([expect.stringMatching(/^resources\/src@.*$/)]);


I wonder how did this test work before? This PR does not seem to affect this behavior, does it?

I'm not sure what exactly, but some change in this PR made filteredStack contain something when testInfo.attach() is called. That leads tracing to include the source code as a resource. Let me refactor this assertion to make that more clear.

there you go: d277551

dgozman · 2024-12-30T09:21:47Z

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

@@ -393,3 +391,48 @@ test('should show custom fixture titles in actions tree', async ({ runUITest })
    /After Hooks[\d.]+m?s/,
  ]);
 });
+
+test('attachments tab shows all but top-level .push attachments', async ({ runUITest }) => {


Sounds fine for now. I see that's already done in the testInfo.ts?

We can follow up by adding a timestamp to attachments and emitting a separate "testend" event in the trace with all the leftover attachments. At that point we can possibly remove "attach foo" steps as well. Up for discussion.

dgozman · 2024-12-30T09:23:25Z

docs/src/test-reporter-api/class-teststep.md

@@ -50,6 +50,16 @@ Start time of this particular test step.

 List of steps inside this step.

+## property: TestStep.attachments


Seeing that every reporter implementation immediately turns this into indices, perhaps we should revisit the API and make this an array of numbers? This does not block the PR, but please put it as an explicit topic for the API review.

packages/playwright/src/worker/testInfo.ts

dgozman · 2024-12-30T09:46:48Z

packages/playwright/src/isomorphic/teleReceiver.ts

@@ -512,8 +516,11 @@ class TeleTestStep implements reporterTypes.TestStep {
  parent: reporterTypes.TestStep | undefined;
  duration: number = -1;
  steps: reporterTypes.TestStep[] = [];
+  attachments: reporterTypes.TestStep['attachments'] = [];


Let's make this a getter and compute lazily upon access? There is a memory pressure problem when merging a lot of large reports, and this class takes the most memory because there are a lot of steps.

I made it a getter in 365b30f. We can look into refactoring the rest of this as well, so that TeleTestStep only keeps _startPayload and _endPayload references.

packages/playwright/src/isomorphic/teleReceiver.ts

github-actions · 2025-01-02T17:12:51Z

Test results for "tests 1"

7 flaky

⚠️ [firefox-page] › page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [playwright-test] › ui-mode-test-setup.spec.ts:98:5 › should show errors in config @macos-latest-node18-1
⚠️ [webkit-library] › library/browsertype-connect.spec.ts:264:5 › launchServer › should support slowmo option @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › library/proxy.spec.ts:93:11 › should proxy local network requests › by default › localhost @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › library/selector-generator.spec.ts:35:5 › selector generator › should prefer button over inner span @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › page/page-set-input-files.spec.ts:205:3 › should upload multiple large files @webkit-ubuntu-22.04-node18
⚠️ [playwright-test] › ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

37498 passed, 650 skipped
✔️✔️✔️

Merge workflow run.

Skn0tt added 2 commits December 16, 2024 17:28

first go

9b33383

anchor by attachment index

6c61ab3

Skn0tt requested a review from dgozman December 16, 2024 17:46

Skn0tt self-assigned this Dec 16, 2024

add one more test

9765124

This comment has been minimized.

Sign in to view

Skn0tt added 2 commits December 16, 2024 19:16

track attachment index in json reporter

9a5e255

recreate attachments in teleReceiver

617dfcc

This comment has been minimized.

Sign in to view

Skn0tt changed the title ~~feat(html reporter): show attachments in attachment steps~~ feat(reporter): report TestStep#attachments Dec 17, 2024

Skn0tt changed the title ~~feat(reporter): report TestStep#attachments~~ feat(reporter): report TestStep#attachments Dec 17, 2024

Skn0tt added 3 commits December 17, 2024 09:58

fix playwright.trace tests

0861d29

fix more tests

b04880d

don't copy screenshot file

1b1da1c

This comment has been minimized.

Sign in to view

pavelfeldman reviewed Dec 20, 2024

View reviewed changes

Skn0tt added 7 commits December 20, 2024 09:57

address first part of feedback

61329a8

use expectZone to get current step for expects

4c345bb

make _attachScreenshot use attach()

bc66311

revert step.attachments

9231cd5

add test to prevent trace viewer regression

3b0f0a9

trace viewer

4fe6927

rename to attachmentIndices

9039219

Skn0tt commented Dec 20, 2024

View reviewed changes

Skn0tt added 2 commits December 20, 2024 13:18

work around trace viewer limitation

dc2d34d

update message

5ea5422

Skn0tt commented Dec 20, 2024

View reviewed changes

revert _attachScreenshot change

9d6abe5

Skn0tt added 2 commits December 20, 2024 15:07

Merge branch 'main' into teststep-attachments

acb0b9c

Merge branch 'main' into teststep-attachments

bf83b70

This comment has been minimized.

Sign in to view

Skn0tt requested a review from pavelfeldman December 20, 2024 15:49

Skn0tt added 2 commits December 20, 2024 16:56

update assertion

866cc81

revert test change

64305ae

This comment has been minimized.

Sign in to view

dgozman reviewed Dec 30, 2024

View reviewed changes

Skn0tt added 3 commits December 30, 2024 11:55

internal error

ba69c19

turn attachments into getter

365b30f

make test change more obvious

d277551

This comment has been minimized.

Sign in to view

Skn0tt added 3 commits December 30, 2024 14:20

when a step times out, there's no "step end" event for it

c86ffbd

Merge branch 'main' into teststep-attachments

2adda21

fix types

f40782d

This comment has been minimized.

Sign in to view

Skn0tt requested a review from dgozman January 2, 2025 10:43

dgozman approved these changes Jan 2, 2025

View reviewed changes

packages/playwright/src/isomorphic/teleReceiver.ts Outdated Show resolved Hide resolved

_result

0fcbabc

Skn0tt merged commit 04a3574 into microsoft:main Jan 2, 2025
29 checks passed

Skn0tt mentioned this pull request Jan 2, 2025

chore: move attachments link to step body #34196

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(reporter): report `TestStep#attachments` #34037

feat(reporter): report `TestStep#attachments` #34037

Skn0tt commented Dec 16, 2024 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Skn0tt Dec 20, 2024

dgozman Dec 30, 2024

Skn0tt Dec 30, 2024

Skn0tt left a comment

This comment has been minimized.

This comment has been minimized.

dgozman Dec 30, 2024

Skn0tt Dec 30, 2024 •

edited

Loading

Skn0tt Dec 30, 2024

dgozman Dec 30, 2024

dgozman Dec 30, 2024

dgozman Dec 30, 2024

Skn0tt Dec 30, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 2, 2025

		@@ -50,6 +50,16 @@ Start time of this particular test step.

		List of steps inside this step.

		## property: TestStep.attachments

feat(reporter): report TestStep#attachments #34037

feat(reporter): report TestStep#attachments #34037

Conversation

Skn0tt commented Dec 16, 2024 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skn0tt left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Skn0tt Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 2, 2025

Test results for "tests 1"

feat(reporter): report `TestStep#attachments` #34037

feat(reporter): report `TestStep#attachments` #34037

Skn0tt commented Dec 16, 2024 •

edited

Loading

Skn0tt Dec 30, 2024 •

edited

Loading