[codex] Fix marker resolution and duplicate mapping validation#63
[codex] Fix marker resolution and duplicate mapping validation#63
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ramilamparo
left a comment
ramilamparo
left a comment
There was a problem hiding this comment.
Looks good. This edge case makes sense to me.
| }) | ||
|
|
||
| test('Should extract fallback marker from spec title, not describe titles', async () => { | ||
| const jsonContent = JSON.stringify({ |
There was a problem hiding this comment.
The newly created JSON files is the same as this. Import it instead.
| run( | ||
| `playwright-json-upload -r ${runURL} ./src/tests/fixtures/playwright-json/describe-marker-unmatched.json` | ||
| ) | ||
| ).rejects.toThrowError() |
There was a problem hiding this comment.
I'm getting deprecation notices, use toThrow instead.
| private resolveMarker(result: TestCaseResult, projectCode: string): TestCaseMarker | null { | ||
| if (result.markerResolution === 'resolved' && result.marker) { | ||
| return result.marker.projectCode.toLowerCase() === projectCode.toLowerCase() | ||
| ? result.marker | ||
| : null | ||
| } | ||
|
|
||
| if (result.markerResolution === 'resolved-none') { | ||
| return null | ||
| } | ||
|
|
||
| if (!result.name) { | ||
| result.markerResolution = 'resolved-none' | ||
| return null | ||
| } | ||
|
|
||
| const seq = this.markerParser.extractSeq(result.name, projectCode) | ||
| if (seq === null) { | ||
| result.markerResolution = 'resolved-none' | ||
| return null | ||
| } | ||
|
|
||
| result.marker = { | ||
| projectCode: this.markerParser.detectProjectCode(result.name) || projectCode, | ||
| seq, | ||
| } | ||
| result.markerResolution = 'resolved' | ||
| return result.marker | ||
| } |
There was a problem hiding this comment.
I think this is still the job of parsers. If we move it to the parsers, we can simplify the code a bit. We can drop markerResolution field and derive this information whether marker field is null or not.
But I understand that the parsers (specifically JUnit) needs context about the project code to correctly derive marker from the test name. We can skip it for now.
satvik007
force-pushed
the
fix/marker-resolution-duplicate-mapping
branch
from
025c707 to
c8094a5
Compare
|
Claude finished @satvik007's task in 11m 34s —— View job Code Review: Fix marker resolution and duplicate mapping validation
SummarySubmitted a 1 new inline comment:
Verified sound and not re-flagged:
Cleanup performed:
|
There was a problem hiding this comment.
Review Summary
Re-reviewed the latest commits (dab04d5, 240fb10, c8094a5). The plan/finalize split for --create-tcases correctly closes the orphan-tcase window: validateDuplicateMappings now runs before any side effect (run creation and test case creation), and assignResolvedTarget correctly suppresses the same-title fan-in case via allowDuplicateTarget. The earlier investigation report files were also removed in 240fb10.
One new finding (1 inline comment):
ResultUploadCommandHandler.ts:188— Subtle behavior change: Playwright reports whose only markers appear indescribetitles will now fail auto-project-code detection (previously they auto-detected the project, even though they would still fail to map results). Suggested an easy relaxation of the gate that preserves the mapping fix while keeping the prior detection UX.
Prior findings still applicable (not re-posted):
resolveMarkercross-project inconsistency at line 597–602- Redundant
resolveMarkerscall at line 135 (aftergetTCaseIdsalready resolved them) - DRY violation between
validateAndPrintDuplicateMappingsandprintDuplicateMappings - Double
getReusableTCasesInDefaultFoldercall (plan + create paths) validateDuplicateMappingsre-iterates results that were already mapped ingetTCaseIds- Missing dedicated unit tests for
mapping.ts(semantics ofallowDuplicateTargetsuppression worth a doc comment too) - Missing end-to-end test for
assignResolvedTargetsame-title fan-in succeeding under--create-tcases - @ramilamparo: dedupe inline JSON in
playwright-json-parsing.spec.ts:527with the new fixture file; replacetoThrowErrorwithtoThrow
Items reviewed and found sound:
- New plan/finalize ordering for
--create-tcases assignResolvedTargetsettingallowDuplicateTarget = trueonly whenresults.length > 1skipUploaderDuplicateValidationlogic (handler validates pre-creation, uploader validates only when reusing an existing run)- Allure and JUnit parsers populating
marker/markerResolutioncorrectly - New regression tests for describe-title collision and unmatched describe-only markers
| if (result.markerResolution === 'resolved' && result.marker?.projectCode) { | ||
| return result.marker.projectCode | ||
| } | ||
| if (result.markerResolution !== 'resolved-none' && result.name) { |
There was a problem hiding this comment.
Behavior change: Playwright auto-project-code detection regresses for files where only describe titles contain markers
Previously, detectProjectCodeFromTCaseNames unconditionally scanned result.name (which for Playwright includes the full prefixed path: "Test Case Folders (TEST-002, TEST-003) › viewer..."), so a project code like TEST could still be auto-detected even when the spec title itself had no marker.
With the new gating on line 188 (result.markerResolution !== 'resolved-none'), the Playwright parser now sets 'resolved-none' for those exact results (because getParsedMarkerFromText(spec.title) ignores the describe-title markers — which is the correct mapping fix). The handler then also skips the name-based fallback for project-code detection. Net effect: a Playwright report whose only marker hints live in describe titles will now bail out with "Could not detect project code…" and require the user to pass --project-code explicitly.
This is arguably correct (those describe markers were always misleading), but it’s a user-visible behavior change not called out in the PR description. Two options:
- Document the change in the PR / release notes so users with such reports know to add
--project-code. - Or relax the gate here so project-code detection (which is non-binding and never decides the actual mapping) can still walk
result.nameeven whenmarkerResolution === 'resolved-none'. e.g.:
if (result.name) {
const code = this.markerParser.detectProjectCode(result.name)
if (code) return code
}Project-code detection only picks the project; it doesn't choose which test case to map to, so loosening it here can't reintroduce the original wrong-mapping bug.
2 participants
Summary
This branch fixes the remaining result-mapping issues in
qas-cliby 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 Playwrightdescribe(...)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-tcasescould still create new test cases before the duplicate rejection fired.What Changed
TestCaseResultspec.titleonly, notdescribeprefixesresolved-nonewhen only thedescribetitle contains markersresolvedstays resolvedresolved-noneis not re-parsed from display namesneeds-project-resolutionstill allows the project-aware JUnit fallback path--forceas the override for duplicate-target failures--create-tcasescreation calls--create-tcaseshandling into a read-only planning step and a post-validation creation/finalization step--forceand mapping behavior describe duplicate-target handling explicitlypackage.jsonand rootpackage-lock.jsonmetadata to0.6.1Why 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:
--create-tcaseswhen duplicate validation failed too lateThe branch makes parser intent explicit and ensures duplicate rejection happens before either of those side effects.
Suggested Review Order
src/utils/result-upload/types.tssrc/utils/result-upload/mapping.tssrc/utils/result-upload/parsers/playwrightJsonParser.tssrc/utils/result-upload/ResultUploadCommandHandler.tssrc/utils/result-upload/ResultUploader.tssrc/tests/playwright-json-parsing.spec.tssrc/tests/result-upload.spec.tsTests Added
describetitles contain unrelated markers butspec.titlecontains the real onedescribetitle contains markers and the result must remain unmatched--create-tcasesValidation
npm run checknpm testNotes
0.6.1