feat(html): link from attachment step to attachment #33267

Skn0tt · 2024-10-24T12:12:40Z

Similar to #33265. Adds a link from the attachment step to the attachment view.

Skn0tt · 2024-10-24T12:40:50Z

demo:

Screen.Recording.2024-10-24.at.14.40.25.mov

dgozman

A screenshot of some sorts would be nice to see.

dgozman · 2024-10-24T14:41:01Z

packages/html-reporter/src/testResultView.tsx

    <span style={{ float: 'right' }}>{msToString(step.duration)}</span>
+    {attachmentName && <a style={{ float: 'right' }} title='link to attachment' href={'#' + componentID(params => params.set('attachment', attachmentName))} onClick={(evt) => { evt.stopPropagation(); }}>{icons.attachment()}</a>}


Does this rely on the browser to automatically scroll to the anchor? How does that work together with the router? I am confused.

Yes, that's how it works. Didn't even know we had a router until now - i'll see if I can switch this to our Link component.

I figured using the anchor to target an element is the most browser-standards way of making this work. But I can also change it to use scrollIntoView if you'd prefer that.

#33295 is a refactoring that allows us to use one scrollIntoView implementation for all attachments. It makes this implementation a lot easier.

packages/html-reporter/src/testResultView.tsx

Skn0tt · 2024-11-19T16:20:00Z

I've updated this PR to use the Anchor primitive we added in #33537.

Skn0tt · 2024-11-19T17:19:37Z

sorry, i'll need to look more into this to make links to trace etc work

Skn0tt · 2024-11-20T09:15:33Z

Here's a demo of it working:

Screen.Recording.2024-11-20.at.10.15.00.mov

dgozman · 2024-11-20T14:22:26Z

packages/html-reporter/src/testResultView.tsx

  }, [result]);

+  const screenshotAnchor = React.useMemo(() => screenshots.map(a => `attachment-${a.name}`), [screenshots]);


screenshotAnchors?

why not return this from the previous useMemo()?

Same for otherAttachmentsAnchor.

That'd also work, i'll make that change.

dgozman · 2024-11-20T14:26:21Z

packages/html-reporter/src/testResultView.tsx

    <span style={{ float: 'right' }}>{msToString(step.duration)}</span>
+    {attachmentName && <a style={{ float: 'right' }} title='link to attachment' href={`#?testId=${test.testId}&anchor=attachment-${encodeURIComponent(attachmentName)}&run=${test.results.indexOf(result)}`} onClick={evt => { evt.stopPropagation(); }}>{icons.attachment()}</a>}


I wonder why do we use <Link> in some places, and plain <a> in others?

Can we simplify link generation to avoid passing test and result around somehow? Perhaps take existing ones from the current url? Or put them into a context? Something for a followup.

no idea 🤷 I guess I can also rewrite this to <Link>, but I don't think there'll be a big difference since it's not a text link but a button

agree, that'd be nice. i'll look at it in a followup.

i've tried rewriting, but that doesn't work as well because <Link> doesn't allow passing in the float: 'right' style. Let's stick with <a>.

dgozman · 2024-12-11T12:51:11Z

packages/html-reporter/src/links.tsx

        onReveal();
    };
    window.addEventListener('popstate', listener);
+
+    // check if we're already on the anchor. required to make it work on page load.
+    listener();


Wouldn't this scroll away on some re-renders when effect is re-registered for whatever reason?

Correct. The effect re-registers when id or onReveal change. We can work against the onReveal change using useCallback, but id might change for example when some UI element is added dynamically and now an enclosing Anchor needs to list more IDs. I'll need to rethink this.

How hard can it be to design a Reveal API in React?

Alright, I ended up reverting to the useEffect-based version in 6b3bad4. That makes the code easier to reason about and also ensures that the page-load works as expected.

In a previous iteration, this approach had the problem that it wouldn't re-reveal an element if the current anchor is navigated to again. This iteration fixes that by making the searchParams an effect dependency. So everytime there's a new searchParams object, even if it contains the same parameter strings, the effect will re-fire.

dgozman · 2024-12-11T12:58:31Z

packages/html-reporter/src/links.tsx

    return () => window.removeEventListener('popstate', listener);
  }, [id, onReveal]);
 }

+export function useIsAnchored(id: AnchorID) {


Should we merge this with useAnchor() and make it return boolean?

No, that would only make it harder to use I'm afraid. This one is good for making CSS dependent on anchor, while the other one is good for calling imperative code on anchor change.

dgozman · 2024-12-11T13:08:28Z

packages/html-reporter/src/testFileView.tsx

+  for (const result of test.results) {
+    for (const attachment of result.attachments) {
+      if (attachment.contentType.startsWith('image/') && !!attachment.name.match(/-(expected|actual|diff)/))
+        return <Link href={`#?testId=${test.testId}&anchor=attachment-${attachment.name}&run=${test.results.indexOf(result)}`} title='View images' className='test-file-badge'>{image()}</Link>;


attachment.name for sure needs escaping. Might as well use URLSearchParams here.

done in 2ae3918

dgozman · 2024-12-11T13:30:00Z

packages/html-reporter/src/testResultView.tsx

    <span style={{ float: 'right' }}>{msToString(step.duration)}</span>
+    {attachmentName && <a style={{ float: 'right' }} title='link to attachment' href={`#?testId=${test.testId}&anchor=attachment-${encodeURIComponent(attachmentName)}&run=${test.results.indexOf(result)}`} onClick={evt => { evt.stopPropagation(); }}>{icons.attachment()}</a>}


Can we extract a helper function for attachmentAnchorURL() and use everywhere? It gets pretty complicated with all the parameters.

done in f04234b

dgozman · 2024-12-11T13:31:00Z

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

+        const navState = new URLSearchParams(url.hash.slice(1));
+        return navState.get('anchor') === 'attachment-foo';
+      });
+    });


Perhaps we can use expect().toBeInViewport() here?

done in 91ef9c6

dgozman · 2024-12-12T16:18:59Z

packages/html-reporter/src/testFileView.tsx

@@ -59,7 +59,7 @@ export const TestFileView: React.FC<React.PropsWithChildren<{
          <span data-testid='test-duration' style={{ minWidth: '50px', textAlign: 'right' }}>{msToString(test.duration)}</span>
        </div>
        <div className='test-file-details-row'>
-          <Link href={`#?testId=${test.testId}`} title={[...test.path, test.title].join(' › ')} className='test-file-path-link'>
+          <Link href={testResultHref({ test })} title={[...test.path, test.title].join(' › ')} className='test-file-path-link'>


I wonder whether testResultHref() should automatically append filterParam? For example, here and below at line 78 we lose it, which looks unintentional.

Sounds good. I'd rather fix that in a separate PR though, so it's easier to revert if we don't want it for some reason.

dgozman · 2024-12-12T16:22:11Z

packages/html-reporter/src/testResultView.tsx

          <ImageDiffView diff={diff}/>
        </AutoChip>
      </Anchor>
    )}

-    {!!screenshots.length && <AutoChip header='Screenshots'>
+    {!!screenshots.length && <Anchor id={screenshotAnchors}><AutoChip header='Screenshots' revealOnAnchorId={screenshotAnchors}>
      {screenshots.map((a, i) => {
        return <div key={`screenshot-${i}`}>


Does it make sense to wrap each screenshot in Anchor to be able to scroll to a particular one?

good catch, added in d098a57

dgozman · 2024-12-16T13:22:48Z

packages/html-reporter/src/testResultView.tsx

          <ImageDiffView diff={diff}/>
        </AutoChip>
      </Anchor>
    )}

-    {!!screenshots.length && <AutoChip header='Screenshots'>
+    {!!screenshots.length && <Anchor id={screenshotAnchors}><AutoChip header='Screenshots' revealOnAnchorId={screenshotAnchors}>


nit: do we need a big Anchor here? I think expanding AutoChip should just work, and each screenshot will be scrolled to by the inner Anchor.

github-actions · 2024-12-16T14:12:59Z

Test results for "tests 1"

1 failed
❌ [playwright-test] › playwright.artifacts.spec.ts:222:5 › should work with screenshot: only-on-failure & fullPage @macos-latest-node18-1

6 flaky

⚠️ [firefox-library] › library/inspector/cli-codegen-aria.spec.ts:77:7 › should update aria snapshot highlight @firefox-ubuntu-22.04-node18
⚠️ [firefox-page] › page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [webkit-library] › library/debug-controller.spec.ts:202:1 › should record custom data-testid @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › library/proxy.spec.ts:63:3 › should work with IP:PORT notion @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › library/trace-viewer.spec.ts:109:1 › should show tracing.group in the action list with location @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

37305 passed, 650 skipped
✔️✔️✔️

Merge workflow run.

feat(html): link from attachment step to attachment

8afe8f6

Skn0tt requested a review from dgozman October 24, 2024 12:12

Skn0tt self-assigned this Oct 24, 2024

Skn0tt force-pushed the sk-branch-4 branch from 54a06df to c060923 Compare October 24, 2024 12:38

This comment has been minimized.

Sign in to view

dgozman reviewed Oct 24, 2024

View reviewed changes

don't plumb category

c89488a

Skn0tt force-pushed the sk-branch-4 branch from c060923 to c89488a Compare October 25, 2024 07:46

This comment has been minimized.

Sign in to view

Skn0tt mentioned this pull request Oct 25, 2024

chore(html): anchor by attachment index, not by attachment type #33295

Closed

fix linter

0afc545

This comment has been minimized.

Sign in to view

remove category

3730900

This comment has been minimized.

Sign in to view

Skn0tt added 3 commits November 19, 2024 17:07

revert componentID

dcf885f

Merge branch 'main' into sk-branch-4

579fbd2

use Anchor

aa2beda

Skn0tt requested a review from dgozman November 19, 2024 16:20

This comment has been minimized.

Sign in to view

Skn0tt marked this pull request as draft November 19, 2024 17:21

Skn0tt added 4 commits November 20, 2024 09:33

update css

a30c651

fix test

1fbb66d

rework attachment revealing

4276d45

make it work on nav

00d3b41

Skn0tt marked this pull request as ready for review November 20, 2024 09:15

This comment has been minimized.

Sign in to view

dgozman reviewed Nov 20, 2024

View reviewed changes

move memos into existing

78120bb

This comment has been minimized.

Sign in to view

Merge branch 'main' into sk-branch-4

fc90c1f

This comment has been minimized.

Sign in to view

Skn0tt requested a review from dgozman December 11, 2024 13:21

dgozman reviewed Dec 11, 2024

View reviewed changes

Skn0tt added 4 commits December 11, 2024 15:18

Merge branch 'main' into sk-branch-4

663f875

refactor to not subscribe

6b3bad4

escape attachment name

2ae3918

abstract href generator

f04234b

This comment has been minimized.

Sign in to view

Skn0tt added 2 commits December 11, 2024 16:22

use toBeInViewport

91ef9c6

remove unneeded import

aa49ca7

This comment has been minimized.

Sign in to view

Skn0tt requested a review from dgozman December 12, 2024 11:26

dgozman reviewed Dec 12, 2024

View reviewed changes

scroll to individual screenshots

d098a57

This comment has been minimized.

Sign in to view

Skn0tt requested a review from dgozman December 16, 2024 10:47

dgozman approved these changes Dec 16, 2024

View reviewed changes

remove big anchor

53890e1

Skn0tt merged commit 512cb36 into microsoft:main Dec 16, 2024
28 of 29 checks passed

This was referenced Dec 16, 2024

feat(reporter): report TestStep#attachments #34037

Merged

fix: don't rely on requestAnimationFrame #34065

Merged

		<span style={{ float: 'right' }}>{msToString(step.duration)}</span>
		{attachmentName && <a style={{ float: 'right' }} title='link to attachment' href={'#' + componentID(params => params.set('attachment', attachmentName))} onClick={(evt) => { evt.stopPropagation(); }}>{icons.attachment()}</a>}

		}, [result]);

		const screenshotAnchor = React.useMemo(() => screenshots.map(a => `attachment-${a.name}`), [screenshots]);

feat(html): link from attachment step to attachment #33267

feat(html): link from attachment step to attachment #33267

Conversation

Skn0tt commented Oct 24, 2024

This comment has been minimized.

Skn0tt commented Oct 24, 2024 • edited Loading

This comment has been minimized.

dgozman left a 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.

Skn0tt commented Nov 19, 2024

This comment has been minimized.

Skn0tt commented Nov 19, 2024

Skn0tt commented Nov 20, 2024

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

Choose a reason for hiding this comment

Skn0tt Nov 20, 2024 • edited Loading

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 11, 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

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.

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.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 16, 2024

Test results for "tests 1"

Skn0tt commented Oct 24, 2024 •

edited

Loading

Skn0tt Nov 20, 2024 •

edited

Loading

Skn0tt Dec 11, 2024 •

edited

Loading