fix(html-reporter): prevent crash when annotation type is missing or non-string #37761

vladlearns · 2025-10-07T21:03:53Z

Defensive change to make sure that if a.type is undefined or any falsy val, it defaults to an empty string before calling .toLowerCase(), preventing the crash

Crash happens here:

vladlearns · 2025-10-07T21:07:21Z

@microsoft-github-policy-service agree

…non-string

…check

….push()

agg23

With a project of this size, we require that users file an issue so we can triage it before evaluating any PRs.

packages/playwright/src/worker/testInfo.ts

packages/html-reporter/src/filter.ts

vladlearns · 2025-10-08T16:35:10Z

With a project of this size, we require that users file an issue so we can triage it before evaluating any PRs.

here you go, please #37771

…merable properties

vladlearns · 2025-10-09T10:45:18Z

I unknowingly copied a buggy pattern that was introduced in Jun 2023 f55bfe2#diff-9fe1fe455371e5592c4ac04ef88de1ae4331ce749bd6333afe8734bb4f7d8b4e for attachments.push. It was latent, because no tests were comparing the attachments arr with toEqual(). When I applied the same pattern to annotations.push, I triggered the bug because the test DOES compare the annotations arr.

Fixed both.

vladlearns · 2025-10-09T14:27:48Z

I unknowingly copied a buggy pattern that was introduced in Jun 2023 f55bfe2#diff-9fe1fe455371e5592c4ac04ef88de1ae4331ce749bd6333afe8734bb4f7d8b4e for attachments.push. It was latent, because no tests were comparing the attachments arr with toEqual(). When I applied the same pattern to annotations.push, I triggered the bug because the test DOES compare the annotations arr.

It's unclear to me what this bug was, even looking at the previously failing test. The change for attachments should be made in a different PR.

I’ve reverted that change in this PR - I’ll open a separate issue explaining why the og pattern is buggy and share the link here /w you once it’s ready.

vladlearns · 2025-10-12T10:16:55Z

I unknowingly copied a buggy pattern that was introduced in Jun 2023 f55bfe2#diff-9fe1fe455371e5592c4ac04ef88de1ae4331ce749bd6333afe8734bb4f7d8b4e for attachments.push. It was latent, because no tests were comparing the attachments arr with toEqual(). When I applied the same pattern to annotations.push, I triggered the bug because the test DOES compare the annotations arr.

It's unclear to me what this bug was, even looking at the previously failing test. The change for attachments should be made in a different PR.

#37813

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

packages/trace-viewer/src/ui/annotationsTab.tsx

packages/playwright/src/common/testType.ts

dgozman · 2025-10-13T13:25:53Z

packages/playwright/src/common/testType.ts

+
+  let description = annotation.description;
+  if (description !== undefined && description !== null && typeof description !== 'string')
+    description = JSON.stringify(description);


Why this one?

#37761 (comment)
I think, for non-string descriptions

packages/playwright/src/worker/testInfo.ts

github-actions · 2025-10-14T10:05:51Z

Test results for "MCP"

2550 passed, 102 skipped

Merge workflow run.

github-actions · 2025-10-14T15:37:13Z

Test results for "tests 1"

2 flaky

⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079 › cli codegen › should not throw csp directive violation errors `@firefox-ubuntu-22.04-node18`
⚠️ [firefox-page] › page/page-emulate-media.spec.ts:144 › should keep reduced motion and color emulation after reload `@firefox-ubuntu-22.04-node18`

45628 passed, 811 skipped

Merge workflow run.

pavelfeldman · 2025-10-17T23:36:40Z

Sorry folks, I'm closing this due to no issue and / or repro steps. I know it is frustrating, but it must start with the issue.

vladlearns · 2025-10-18T08:17:42Z

@pavelfeldman the issue is here: #37771

I even created it on req: #37761 (comment)

vladlearns · 2025-10-18T08:21:08Z

Also, @dgozman and @agg23 I've addressed all of your comments. Please re-review

dgozman · 2025-10-21T16:06:24Z

@vladlearns Sorry, there was a misunderstanding here!

pavelfeldman · 2025-10-21T18:23:13Z

PR should start with the issue, this is now addressed, so we can proceed.

Looking at it again, a couple of thoughts:

You are validating one of the user-provided objects out of tens or hundreds. It improves this exact situation but does not really change the overall validation story. Also it does so in an ad-hock manner.
If we want to improve the user-objects validation we should start using zod and not produce dozens of lines of object validation.
I don't think it is fair for us to ask you to do the first path of these validations and honestly, I don't think the review would converge if we do. But I don't think we should introduce these dozens of lines of ad-hock validation for this one property at the same time.

With that I'm inclined to close the PR.

vladlearns · 2025-10-21T19:48:20Z

PR should start with the issue, this is now addressed, so we can proceed.

Looking at it again, a couple of thoughts:

You are validating one of the user-provided objects out of tens or hundreds. It improves this exact situation but does not really change the overall validation story. Also it does so in an ad-hock manner.

If we want to improve the user-objects validation we should start using zod and not produce dozens of lines of object validation.

I don't think it is fair for us to ask you to do the first path of these validations and honestly, I don't think the review would converge if we do. But I don't think we should introduce these dozens of lines of ad-hock validation for this one property at the same time.

With that I'm inclined to close the PR.

yeah, this is a much better approach. Let's use zod. I will close this one.

vladlearns · 2025-10-21T22:25:26Z

ref: #37959