From e5c086e483299135b8ef74c925628fd4e67e9ee0 Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Fri, 25 Oct 2024 15:14:38 +0200 Subject: [PATCH] chore(html): anchor by attachment index, not by attachment type --- packages/html-reporter/src/chip.tsx | 4 +- packages/html-reporter/src/links.tsx | 5 +- packages/html-reporter/src/reportView.tsx | 2 +- .../html-reporter/src/testCaseView.spec.tsx | 10 ++-- packages/html-reporter/src/testCaseView.tsx | 4 +- packages/html-reporter/src/testFileView.tsx | 23 +++++--- packages/html-reporter/src/testResultView.tsx | 57 ++++++++++++------- tests/playwright-test/reporter-html.spec.ts | 5 +- 8 files changed, 70 insertions(+), 40 deletions(-) diff --git a/packages/html-reporter/src/chip.tsx b/packages/html-reporter/src/chip.tsx index 8f4badf97f..90b0bd2dc4 100644 --- a/packages/html-reporter/src/chip.tsx +++ b/packages/html-reporter/src/chip.tsx @@ -28,7 +28,7 @@ export const Chip: React.FC<{ setExpanded?: (expanded: boolean) => void, children?: any, dataTestId?: string, - targetRef?: React.RefObject, + targetRef?: React.RefCallback, }> = ({ header, expanded, setExpanded, children, noInsets, dataTestId, targetRef }) => { return
, + targetRef?: React.RefCallback, }> = ({ header, initialExpanded, noInsets, children, dataTestId, targetRef }) => { const [expanded, setExpanded] = React.useState(initialExpanded || initialExpanded === undefined); return = ({ attachment, href, linkName, openInNewTab }) => { - return + targetRef?: React.RefCallback, +}> = ({ attachment, href, targetRef, linkName, openInNewTab }) => { + return {attachment.contentType === kMissingContentType ? icons.warning() : icons.attachment()} {attachment.path && {linkName || attachment.name}} {!attachment.path && ( diff --git a/packages/html-reporter/src/reportView.tsx b/packages/html-reporter/src/reportView.tsx index 796c03d6a1..065ced4db1 100644 --- a/packages/html-reporter/src/reportView.tsx +++ b/packages/html-reporter/src/reportView.tsx @@ -76,7 +76,7 @@ const TestCaseViewLoader: React.FC<{ const searchParams = new URLSearchParams(window.location.hash.slice(1)); const [test, setTest] = React.useState(); const testId = searchParams.get('testId'); - const anchor = (searchParams.get('anchor') || '') as 'video' | 'diff' | ''; + const anchor = searchParams.has('anchor') ? Number(searchParams.get('anchor')!) : undefined; const run = +(searchParams.get('run') || '0'); const testIdToFileIdMap = React.useMemo(() => { diff --git a/packages/html-reporter/src/testCaseView.spec.tsx b/packages/html-reporter/src/testCaseView.spec.tsx index 7c9c99eeb3..32b301926c 100644 --- a/packages/html-reporter/src/testCaseView.spec.tsx +++ b/packages/html-reporter/src/testCaseView.spec.tsx @@ -63,7 +63,7 @@ const testCase: TestCase = { }; test('should render test case', async ({ mount }) => { - const component = await mount(); + const component = await mount(); await expect(component.getByText('Annotation text', { exact: false }).first()).toBeVisible(); await expect(component.getByText('Hidden annotation')).toBeHidden(); await component.getByText('Annotations').click(); @@ -79,7 +79,7 @@ test('should render test case', async ({ mount }) => { test('should render copy buttons for annotations', async ({ mount, page, context }) => { await context.grantPermissions(['clipboard-read', 'clipboard-write']); - const component = await mount(); + const component = await mount(); await expect(component.getByText('Annotation text', { exact: false }).first()).toBeVisible(); await component.getByText('Annotation text', { exact: false }).first().hover(); await expect(component.locator('.test-case-annotation').getByLabel('Copy to clipboard').first()).toBeVisible(); @@ -108,7 +108,7 @@ const annotationLinkRenderingTestCase: TestCase = { }; test('should correctly render links in annotations', async ({ mount }) => { - const component = await mount(); + const component = await mount(); const firstLink = await component.getByText('https://playwright.dev/docs/intro').first(); await expect(firstLink).toBeVisible(); @@ -166,7 +166,7 @@ const attachmentLinkRenderingTestCase: TestCase = { }; test('should correctly render links in attachments', async ({ mount }) => { - const component = await mount(); + const component = await mount(); await component.getByText('first attachment').click(); const body = await component.getByText('The body with https://playwright.dev/docs/intro link'); await expect(body).toBeVisible(); @@ -175,7 +175,7 @@ test('should correctly render links in attachments', async ({ mount }) => { }); test('should correctly render links in attachment name', async ({ mount }) => { - const component = await mount(); + const component = await mount(); const link = component.getByText('attachment with inline link').locator('a'); await expect(link).toHaveAttribute('href', 'https://github.com/microsoft/playwright/issues/31284'); await expect(link).toHaveText('https://github.com/microsoft/playwright/issues/31284'); diff --git a/packages/html-reporter/src/testCaseView.tsx b/packages/html-reporter/src/testCaseView.tsx index e3656bf414..7c27c741a5 100644 --- a/packages/html-reporter/src/testCaseView.tsx +++ b/packages/html-reporter/src/testCaseView.tsx @@ -31,7 +31,7 @@ import { CopyToClipboardContainer } from './copyToClipboard'; export const TestCaseView: React.FC<{ projectNames: string[], test: TestCase | undefined, - anchor: 'video' | 'diff' | '', + anchor?: number, run: number, }> = ({ projectNames, test, run, anchor }) => { const [selectedResultIndex, setSelectedResultIndex] = React.useState(run); @@ -69,7 +69,7 @@ export const TestCaseView: React.FC<{ test.results.map((result, index) => ({ id: String(index), title:
{statusIcon(result.status)} {retryLabel(index)}
, - render: () => + render: () => })) || []} selectedTab={String(selectedResultIndex)} setSelectedTab={id => setSelectedResultIndex(+id)} />}
; }; diff --git a/packages/html-reporter/src/testFileView.tsx b/packages/html-reporter/src/testFileView.tsx index bd402a74de..c3bfeeecce 100644 --- a/packages/html-reporter/src/testFileView.tsx +++ b/packages/html-reporter/src/testFileView.tsx @@ -14,7 +14,7 @@ limitations under the License. */ -import type { HTMLReport, TestCaseSummary, TestFileSummary } from './types'; +import type { HTMLReport, TestAttachment, TestCaseSummary, TestFileSummary, TestResultSummary } from './types'; import * as React from 'react'; import { hashStringToInt, msToString } from './utils'; import { Chip } from './chip'; @@ -70,16 +70,25 @@ export const TestFileView: React.FC; }; -function imageDiffBadge(test: TestCaseSummary): JSX.Element | undefined { - const resultWithImageDiff = test.results.find(result => result.attachments.some(attachment => { - return attachment.contentType.startsWith('image/') && !!attachment.name.match(/-(expected|actual|diff)/); +function findResultWithAttachment(test: TestCaseSummary, predicate: (attachment: TestAttachment) => boolean): { index: number, anchor: number } | undefined { + let anchor = 0; + const index = test.results.findIndex(result => result.attachments.some((attachment, i) => { + if (predicate(attachment)) { + anchor = i; + return true; + } })); - return resultWithImageDiff ? {image()} : undefined; + return index === -1 ? undefined : { index, anchor }; +} + +function imageDiffBadge(test: TestCaseSummary): JSX.Element | undefined { + const resultWithImageDiff = findResultWithAttachment(test, attachment => attachment.contentType.startsWith('image/') && !!attachment.name.match(/-(expected|actual|diff)/)); + return resultWithImageDiff ? {image()} : undefined; } function videoBadge(test: TestCaseSummary): JSX.Element | undefined { - const resultWithVideo = test.results.find(result => result.attachments.some(attachment => attachment.name === 'video')); - return resultWithVideo ? {video()} : undefined; + const resultWithVideo = findResultWithAttachment(test, attachment => attachment.name === 'video'); + return resultWithVideo ? {video()} : undefined; } function traceBadge(test: TestCaseSummary): JSX.Element | undefined { diff --git a/packages/html-reporter/src/testResultView.tsx b/packages/html-reporter/src/testResultView.tsx index bb18422dd0..21c1c7c0a6 100644 --- a/packages/html-reporter/src/testResultView.tsx +++ b/packages/html-reporter/src/testResultView.tsx @@ -14,7 +14,7 @@ limitations under the License. */ -import type { TestAttachment, TestCase, TestResult, TestStep } from './types'; +import type { TestAttachment, TestResult, TestStep } from './types'; import * as React from 'react'; import { TreeItem } from './treeItem'; import { msToString } from './utils'; @@ -61,10 +61,24 @@ function groupImageDiffs(screenshots: Set): ImageDiff[] { return [...snapshotNameToImageDiff.values()]; } +function getDiffAttachmentRepresentative(diff: ImageDiff, result: TestResult): TestAttachment { + let representative: TestAttachment | undefined = undefined; + let min = Number.MAX_VALUE; + for (const attachment of [diff.actual?.attachment, diff.diff?.attachment, diff.expected?.attachment]) { + if (!attachment) + continue; + const index = result.attachments.indexOf(attachment); + if (index < min) { + min = index; + representative = attachment; + } + } + return representative!; +} + export const TestResultView: React.FC<{ - test: TestCase, result: TestResult, - anchor: 'video' | 'diff' | '', + anchor?: number, }> = ({ result, anchor }) => { const { screenshots, videos, traces, otherAttachments, diffs, errors, htmls } = React.useMemo(() => { @@ -80,19 +94,24 @@ export const TestResultView: React.FC<{ return { screenshots: [...screenshots], videos, traces, otherAttachments, diffs, errors, htmls }; }, [result]); - const videoRef = React.useRef(null); - const imageDiffRef = React.useRef(null); - - const [scrolled, setScrolled] = React.useState(false); + const attachmentRefs = React.useRef([]); React.useEffect(() => { - if (scrolled) + if (anchor === undefined) return; - setScrolled(true); - if (anchor === 'video') - videoRef.current?.scrollIntoView({ block: 'start', inline: 'start' }); - if (anchor === 'diff') - imageDiffRef.current?.scrollIntoView({ block: 'start', inline: 'start' }); - }, [scrolled, anchor, setScrolled, videoRef]); + + const attachmentRef = attachmentRefs.current[anchor]; + if (!attachmentRef) + return; + + attachmentRef.scrollIntoView({ block: 'start', inline: 'start' }); + }, [attachmentRefs, anchor]); + const targetRef = (attachment: TestAttachment): React.RefCallback => el => { + const index = result.attachments.indexOf(attachment); + if (el) + attachmentRefs.current[index] = el; + else + delete attachmentRefs.current[index]; + }; return
{!!errors.length && @@ -107,7 +126,7 @@ export const TestResultView: React.FC<{ } {diffs.map((diff, index) => - + )} @@ -118,7 +137,7 @@ export const TestResultView: React.FC<{ - +
; })} } @@ -132,12 +151,12 @@ export const TestResultView: React.FC<{
} } - {!!videos.length && + {!!videos.length && {videos.map((a, i) =>
- +
)} } @@ -145,7 +164,7 @@ export const TestResultView: React.FC<{ {[...htmls].map((a, i) => ( ) )} - {[...otherAttachments].map((a, i) => )} + {[...otherAttachments].map((a, i) => )} } ; }; diff --git a/tests/playwright-test/reporter-html.spec.ts b/tests/playwright-test/reporter-html.spec.ts index 11169fadff..2861f497bf 100644 --- a/tests/playwright-test/reporter-html.spec.ts +++ b/tests/playwright-test/reporter-html.spec.ts @@ -175,8 +175,9 @@ for (const useIntermediateMergeReport of [true, false] as const) { expect(result.failed).toBe(1); await showReport(); - await page.click('text=fails'); - await expect(page.locator('text=Image mismatch')).toBeVisible(); + + await page.locator('.test-file-test').filter({ hasText: 'fails' }).getByTitle('View images').click(); + await expect(page.locator('text=Image mismatch')).toBeInViewport(); await expect(page.locator('text=Snapshot mismatch')).toHaveCount(0); await expect(page.getByTestId('test-screenshot-error-view').getByTestId('test-result-image-mismatch-tabs').locator('div')).toHaveText([