Skip to content

[codex] Fix marker resolution and duplicate mapping validation#63

Draft
satvik007 wants to merge 4 commits intomainfrom
fix/marker-resolution-duplicate-mapping
Draft

[codex] Fix marker resolution and duplicate mapping validation#63
satvik007 wants to merge 4 commits intomainfrom
fix/marker-resolution-duplicate-mapping

Conversation

@satvik007
Copy link
Copy Markdown
Collaborator

@satvik007 satvik007 commented Apr 6, 2026

Summary

This branch fixes the remaining result-mapping issues in qas-cli by making parsed marker metadata the source of truth for upload mapping and by validating duplicate target mappings earlier in the flow.

The branch also bumps the package version to 0.6.1.

Root Cause

The previous uploader logic inferred identity from result.name, which is a display field and may include Playwright describe(...) titles. When those describe titles contained multiple QA Sphere markers, the uploader could map a result to the wrong run test case simply because that marker appeared earlier in the string.

There was also a second safety issue after duplicate-target detection was introduced: duplicate validation could still happen after side effects had already occurred. The initial fix moved that check ahead of run creation, but --create-tcases could still create new test cases before the duplicate rejection fired.

What Changed

  • Added explicit marker metadata and resolution state to TestCaseResult
  • Added shared mapping helpers so handler and uploader use the same marker-based mapping rules
  • Switched uploader mapping from name scanning to explicit marker resolution
  • Updated Playwright parsing to:
    • fan out one result per annotation marker
    • extract fallback markers from spec.title only, not describe prefixes
    • preserve resolved-none when only the describe title contains markers
  • Updated JUnit and Allure parsing to populate marker metadata directly where possible
  • Updated handler resolution logic so parser decisions are authoritative:
    • resolved stays resolved
    • resolved-none is not re-parsed from display names
    • needs-project-resolution still allows the project-aware JUnit fallback path
  • Added duplicate-target detection based on resolved target test cases instead of string matching
  • Kept --force as the override for duplicate-target failures
  • Moved duplicate validation ahead of side effects for implicit uploads:
    • before run creation
    • before --create-tcases creation calls
  • Split --create-tcases handling into a read-only planning step and a post-validation creation/finalization step
  • Updated CLI/README text so --force and mapping behavior describe duplicate-target handling explicitly
  • Bumped package.json and root package-lock.json metadata to 0.6.1

Why Reviewers Should Care

This is primarily a correctness fix.

Without these changes, a Playwright suite could upload results to the wrong QA Sphere test cases even when the result count looked plausible. In failure cases, the CLI could also leave behind unwanted artifacts:

  • orphaned runs from post-create duplicate rejection
  • orphaned test cases from --create-tcases when duplicate validation failed too late

The branch makes parser intent explicit and ensures duplicate rejection happens before either of those side effects.

Suggested Review Order

  1. src/utils/result-upload/types.ts
  2. src/utils/result-upload/mapping.ts
  3. src/utils/result-upload/parsers/playwrightJsonParser.ts
  4. src/utils/result-upload/ResultUploadCommandHandler.ts
  5. src/utils/result-upload/ResultUploader.ts
  6. src/tests/playwright-json-parsing.spec.ts
  7. src/tests/result-upload.spec.ts

Tests Added

  • Playwright regression where describe titles contain unrelated markers but spec.title contains the real one
  • Playwright regression where only the describe title contains markers and the result must remain unmatched
  • Upload regression proving duplicate-target mappings fail before creating a new run
  • Upload regression proving duplicate-target mappings fail before creating new test cases under --create-tcases
  • Existing duplicate-target and force-path coverage retained

Validation

  • npm run check
  • npm test

Notes

  • Follow-up package version in this branch: 0.6.1

@claude
Copy link
Copy Markdown

claude bot commented Apr 6, 2026

Claude finished @satvik007's task in 3m 10s —— View job


Code Review: Fix marker resolution and duplicate mapping validation

  • Read review instructions
  • Gather context and read changed files
  • Analyze code changes for correctness, bugs, and best practices
  • Post detailed review feedback

Review submitted. The PR is a well-motivated correctness fix with solid test coverage. I left 5 inline comments:

  • 1 medium-priority issue: resolveMarker leaves markerResolution in an inconsistent state when a marker targets a different project — suggested fix included
  • 3 minor observations (non-blocking): DRY violation in duplicate-mapping formatting, double API call in create-tcases flow, and investigation report files that may not belong in the repo permanently
  • 1 positive note: Clean design of the shared mapping.ts module

gemini-code-assist[bot]

This comment was marked as outdated.

claude[bot]

This comment was marked as outdated.

@claude
Copy link
Copy Markdown

claude bot commented Apr 6, 2026

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


  • Read review instructions
  • Gather PR context and diff
  • Launch specialized review subagents (code quality, security, performance, test coverage)
  • Aggregate findings, deduplicate against existing comments, and post review
  • Clean up outdated bot comments

Summary

Submitted a review with 2 inline comments after deduplicating against 6 existing bot comments:

  1. mapping.ts — Missing unit tests for the core mapping logic (mapResolvedResultsToTCases, findDuplicateTCaseMappings, allowDuplicateTarget suppression). These are the centerpiece of the correctness fix and deserve direct test coverage.
  2. ResultUploadCommandHandler.ts:602 — Reinforced the previous review's suggestion to update markerResolution to 'resolved-none' when the marker targets a different project (currently left inconsistent).

Cleanup performed:

  • Resolved the outdated QAS_CLI_ISSUES_BRIEF.md review thread (files removed in latest commit)
  • Minimized 2 older bot review summaries

@satvik007 satvik007 requested a review from ramilamparo April 6, 2026 07:38
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.

Review Summary (re-review after sync)

The investigation report files have been removed — good. The core changes remain well-structured and the test coverage for the key regression scenarios is solid.

Previous findings still applicable:

  • resolveMarker cross-project inconsistency (line 602) — reinforced with suggestion
  • Redundant resolveMarkers call at line 135 (flagged by gemini)
  • DRY violation in duplicate-mapping error formatting (handler vs uploader)
  • Double getReusableTCasesInDefaultFolder API call in create-tcases flow

New finding:

  • The shared mapping.ts module — which contains the core correctness fix — has no dedicated unit tests. The allowDuplicateTarget suppression logic in findDuplicateTCaseMappings is particularly worth testing directly.

Overall this is a solid correctness fix. The architecture of explicit marker metadata flowing through the pipeline is a clear improvement over name-scanning.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing unit tests for the core mapping logic

mapResolvedResultsToTCases and findDuplicateTCaseMappings are the centerpiece of this PR's correctness fix, but they're only exercised indirectly through end-to-end integration tests in result-upload.spec.ts. A dedicated mapping.spec.ts would make the logic easier to verify and would catch regressions more precisely.

Suggested cases:

  • Results with matching project code + seq go to results; non-matching or marker-less go to missing
  • Case-insensitive project code comparison (prj vs PRJ)
  • Two results mapping to the same tcase.id are reported as duplicates
  • The allowDuplicateTarget suppression: a group where all results have allowDuplicateTarget: true is not reported (this is the key behavior from assignResolvedTarget when multiple results share the same create-tcases title)
  • Mixed case: one result with allowDuplicateTarget: true and one without → still flagged

return result.marker.projectCode.toLowerCase() === projectCode.toLowerCase()
? result.marker
: null
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

+1 to the previous review's suggestion here — when the marker targets a different project, markerResolution should be updated to 'resolved-none' so the state is self-consistent. The current code works because mapResolvedResultsToTCases independently checks result.marker.projectCode, but leaving markerResolution === 'resolved' with a cross-project marker is a latent inconsistency.

Suggested change
}
private resolveMarker(result: TestCaseResult, projectCode: string): TestCaseMarker | null {
if (result.markerResolution === 'resolved' && result.marker) {
if (result.marker.projectCode.toLowerCase() === projectCode.toLowerCase()) {
return result.marker
}
// Marker exists but targets a different project — not resolvable here
result.markerResolution = 'resolved-none'
return null
}

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.

1 participant