Repository navigation
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The new regression case is useful, but it duplicates the same insert_dynamic_tag helper already added by seven open PRs touching this test file, without recording the agreed landing order.
What I ran
Reviewed head c8750a1afe5bd36d84eff418c4c9f12a380cf9d9 against base de824c2919ca65086c6324a1e0388866a7327eea; the no-build preview merged cleanly against current main. Environment: Linux x86_64, GCC 12.2.0, Python 3.11.2. No build or relinker-backed test was run.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 13741
python3 tools/pr_test_quality.py --root /marc/AnyPS5 --json core/relinker/relinker/tests/test_guest_none_relocations.pyResults
| Check | Result |
|---|---|
| Diff and conventions | Passed. |
| Python syntax and test registration | Passed in precheck. |
| Test-quality heuristic | No findings. |
| Merge preview | Clean against current main; the stack report found no declared dependencies. This does not detect the duplicate helper across the live PRs. |
| Hosted CI | Eligibility, conventions, intake, and labels were queued at the final check. |
| Author-reported tests | The PR reports guest_none_relocations passing on macOS arm64 and 526/526 relinker-only tests. I did not reproduce these runs. |
| Precheck helper | Its isolated checkout could not locate pr_test_quality.py; I reran that helper from the workspace. This was a harness issue, not a PR failure. |
Selected unedited check output
All conventions checks passed.
230 test script(s), each run by an add_test or a workflow
{
"findings": [],
"negative_control_candidates": [],
"limits": [
"Heuristics can miss weak assertions and can flag valid tests; inspect every finding.",
"A test that passes against base is not by itself proof that the PR test is inadequate.",
"Negative controls are only candidates until the test actually runs against base and fails by assertion rather than import/setup error."
]
}/usr/bin/python3: can't open file '/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-13741-c8750a1afe5b/head-c8750a1afe5b-b9b45bd2/tools/pr_test_quality.py': [Errno 2] No such file or directory
Conclusion: checks failed
Negative control
I did not run a negative control. The new Python test invokes a relinker binary, and no exact-head build was performed; I could not remove the duplicate-tag rejection and observe the test fail.
Findings
Blocking
The added insert_dynamic_tag function is identical to the helper in open PRs #13723, #13727, #13729, #13731, #13733, #13739, and #13740, all of which modify core/relinker/relinker/tests/test_guest_none_relocations.py. The PR description acknowledges this family shares the test file, but the checklist says Depends on: none. Disjoint invalid-case names do not resolve the helper overlap: whichever branch lands later can conflict or reintroduce a duplicate definition. The repository requires authors to agree on landing order for overlapping PRs and record the dependency.
Please coordinate which PR supplies the shared helper first. If #13741 is first, record the agreed order and update the later PRs accordingly; otherwise, remove this duplicate helper and depend on the PR that introduces the shared implementation. Set the checklist's Depends on line to the agreed PR number(s).
Non-blocking
The new test covers the same duplicate STRTAB rejection in plain ELF and SCE-tagged fixtures, and the reported test run is useful evidence. No separate behavioral defect was found in this added case.
Sources
- This PR's helper and duplicate-tag case at its reviewed head
- The same helper at the current heads of the other open PRs: #13723, #13727, #13729, #13731, #13733, #13739, and #13740.
- Repository overlap-order and
Depends onrule at the PR base - PR description with the author-reported test results
Not verified
I did not build the relinker or run ctest; the author-reported test results were not independently reproduced.
To get this to a good state
Please agree on the helper's landing order, record the corresponding dependency, and remove or reuse the duplicate helper as appropriate. I will rerun the same checks on the updated head.
|
Consolidated into #14079, which adds all of these guest_none_relocations invalid cases in one change so the test file has a single open pull request. Closing this per-case pull request in favour of that one. |
What
GuestImageReaderrejects a second occurrence of the same mapped dynamic tag withDuplicate dynamic tag <n>. This adds one invalid-case harness entry that inserts a duplicate STRTAB tag (5 /0x61000035) for both plain ELF and SCE-tagged fixtures, using the existing reader message.Tested
macOS arm64, upstream/main
de824c2919, Python 3.13, relinker-only Release-DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON:ctest -R guest_none_relocations: pass (newduplicate-dynamic-tag-strtabforsce=False/True).ctest -j 8 -E vulkan_validation_gate: 526/526.python3 tools/check_conventions.py --base upstream/main: passed.python3 tools/pr_overlap.py --branch test/duplicate-guest-dynamic-tag: no overlap with open PRs (merge-order with test(relinker): pin invalid guest lifecycle function relocation #13711–test(relinker): pin unsupported guest dynamic tag 22 #13740 family on the same test file; disjoint invalid-case names).Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)