Skip to content

Fix incorrect Playwright upload mapping to QA Sphere runs#61

Open
ramilamparo wants to merge 6 commits intomainfrom
fix/playwright-upload-mapping
Open

Fix incorrect Playwright upload mapping to QA Sphere runs#61
ramilamparo wants to merge 6 commits intomainfrom
fix/playwright-upload-mapping

Conversation

@ramilamparo
Copy link
Copy Markdown
Collaborator

Summary

Fixes three bugs that caused playwright-json-upload to map results to wrong QA Sphere test cases, producing misleading run summaries (e.g., 33 passed / 1 failed instead of 140 passed / 2 failed):

  • Exact marker matching: nameMatchesTCase() used substring .includes(), so QS1-104 incorrectly matched QS1-10427. Replaced with \b word-boundary regex.
  • Multi-annotation fan-out: Playwright parser only used the first test case annotation per test, dropping additional ones. Now produces one TestCaseResult per annotation.
  • Upload message clarity: Changed "Uploaded N test cases" to "Uploaded X results to Y test cases" so discrepancies between result count and target test case count are visible.
  • Attachment deduplication: When multiple results reference the same file (common with fan-out and shared attachments), each unique file is now uploaded once and the URL reused. Added filePath to Attachment type for dedup keying. Progress shows "(N duplicates skipped)".

Test plan

  • nameMatchesTCase prefix collision tests (QS1-104 vs QS1-10427)
  • Multi-annotation fan-out parsing tests (single, multiple, precedence over name markers)
  • Upload message format test with many-to-one mapping (4 results → 3 test cases)
  • Attachment dedup tests across all 3 parsers (matching-tcases fixtures: 2 unique + 3 dupes)
  • Attachment dedup test for Playwright fan-out (multi-annotation-with-attachments fixture)
  • All 218 tests pass, build clean, lint/format clean

Closes Hypersequent/tms-issues#2394

🤖 Generated with Claude Code

Three bugs caused Playwright test results to map to wrong QA Sphere
test cases, producing misleading run summaries:

1. Marker matching used substring includes, so QS1-104 incorrectly
   matched QS1-10427. Replace with word-boundary regex.

2. Playwright parser only used the first "test case" annotation per
   test, dropping additional ones. Now fans out one TestCaseResult
   per annotation.

3. Upload message said "Uploaded N test cases" conflating result
   count with target test case count. Now reports both:
   "Uploaded X results to Y test cases".

Additionally, deduplicate file uploads when multiple results reference
the same attachment. Previously each result uploaded its own copy.
Now uploads each unique file once and reuses the URL. Progress message
shows duplicates skipped count.

Closes Hypersequent/tms-issues#2394
@claude

This comment was marked as resolved.

@ramilamparo ramilamparo force-pushed the fix/playwright-upload-mapping branch from 86df41e to 44ea8dd Compare April 1, 2026 10:14
chatgpt-codex-connector[bot]

This comment was marked as outdated.

@claude

This comment was marked as outdated.

gemini-code-assist[bot]

This comment was marked as outdated.

@ramilamparo ramilamparo force-pushed the fix/playwright-upload-mapping branch from 44ea8dd to 86df41e Compare April 1, 2026 10:18
claude[bot]

This comment was marked as outdated.

@claude

This comment was marked as outdated.

@claude
Copy link
Copy Markdown

claude bot commented Apr 1, 2026

Claude finished @ramilamparo's task in 4m 33s —— View job


Claude finished reviewing this PR


  • Gather PR context and diff
  • Launch specialized review subagents
  • Aggregate findings and post review
  • Clean up outdated bot comments

Summary

Good PR — the four fixes (exact marker matching, multi-annotation fan-out, upload message clarity, attachment dedup) are correct and well-tested. Posted two new inline findings:

  1. Shared attachment array reference (playwrightJsonParser.ts:136-139) — fan-out entries share the same resolved Attachment[] by reference. Safe today but fragile.
  2. filePath not normalized for dedup (utils.ts:29) — equivalent paths like ./foo vs foo won't deduplicate.

Minimized 3 outdated bot review bodies and resolved 1 duplicate bot thread. The existing codex comment on \b word-boundary behavior with underscores and the prior claude comment on annotation dedup are still relevant and were kept.

claude[bot]

This comment was marked as outdated.

@claude

This comment was marked as resolved.

@claude

This comment was marked as outdated.

claude[bot]

This comment was marked as outdated.

@ramilamparo ramilamparo requested review from a team, hi-rai and satvik007 April 1, 2026 10:50
@claude
Copy link
Copy Markdown

claude bot commented Apr 1, 2026

Claude finished @ramilamparo's task in 4m 52s —— View job


  • Gather PR context and diff
  • Launch specialized review subagents
  • Aggregate findings and post review
  • Clean up outdated bot comments

Review Summary

Good PR — the four fixes (exact marker matching, multi-annotation fan-out, upload message clarity, attachment dedup) are correct and well-tested. One new inline finding posted; addressed threads from prior reviews resolved.

New Finding

  • Unescaped attachment filename in HTML (ResultUploader.ts:407) — makeListHtml interpolates item.name (from basename(filePath)) into HTML without escaping. The codebase already uses escape-html in all three parsers; it should be applied here too.

Addressed (resolved)

  • \b word boundary with underscores (codex) — no longer applies; code now uses (?<![a-zA-Z0-9])...(?!\d) which handles underscores correctly (confirmed by tests)
  • No annotation dedup — addressed; code now uses [...new Set(markers)]
  • Shared attachment array reference — addressed; code now clones with [...tcaseAttachment]
  • filePath not normalized — addressed; code uses resolve() which normalizes paths

Minimized 4 outdated bot review bodies.

Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good PR — the four fixes (exact marker matching, multi-annotation fan-out, upload message clarity, attachment dedup) are correct and well-tested. The asymmetric regex boundary ((?<![a-zA-Z0-9]) lookbehind + (?!\d) lookahead) is a good design choice for the prefix-collision problem while keeping underscore/bracket-adjacent markers working.

One new inline finding on unescaped HTML in makeListHtml. Prior inline comments on annotation dedup, shared attachment array, and \b underscore behavior have all been addressed by commits in this PR — the code now uses [...new Set(markers)] for dedup, [...tcaseAttachment] for cloning, and (?<![a-zA-Z0-9])...(?!\d) instead of \b.

@claude
Copy link
Copy Markdown

claude bot commented Apr 1, 2026

Claude finished @ramilamparo's task in 4m 4s —— View job


  • Read review instructions
  • Gather PR context and diff
  • Launch specialized review subagents
  • Aggregate findings and post review
  • Clean up outdated bot comments

Review Summary

Good PR — all four fixes (exact marker matching, multi-annotation fan-out, upload message clarity, attachment dedup) are correct and well-tested. All prior review feedback has been addressed in the latest commits:

  • Annotation dedup[...new Set(markers)]
  • Shared attachment array[...tcaseAttachment] clone
  • filePath normalizationresolve() for dedup keying
  • escapeHtml in makeListHtml → applied to both name and url
  • \b underscore behavior → replaced with asymmetric (?<![a-zA-Z0-9])...(?!\d) which correctly handles underscores/brackets (confirmed by tests)

No new noteworthy issues found. Minimized 5 outdated bot review summaries.

Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good PR — all four fixes are correct, well-motivated, and well-tested. Prior review feedback (annotation dedup, shared array cloning, filePath normalization via resolve(), escapeHtml in makeListHtml, asymmetric regex boundary) has all been addressed in the latest commits.

No new noteworthy issues found. The code is ready to merge.

@satvik007
Copy link
Copy Markdown
Collaborator

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants