Repository navigation
test(relinker): consolidate guest_none_relocations invalid cases - #14079
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The new macOS invalid-ELF cases call the converter but never assert its exit status, error message, or output files, so they cannot fail when the invalid image is accepted.
What I ran
Reviewed exact head 1590be17ddb643eecb39a1992e38fc1ba8ca0b87 against base ca073ec3ad0d766ea9f4658e418a5c977001a353. The no-build merge preview was clean. Environment: Linux x86-64, Python 3.11.2, Git 2.39.5. I did not build the relinker.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14079 --output-dir /tmp/anyps5-14079-review
gh api repos/boykopovar/AnyPS5/contents/core/relinker/relinker/tests/test_guest_none_relocations.py?ref=1590be17ddb643eecb39a1992e38fc1ba8ca0b87Results
| Check | Result |
|---|---|
| Diff check | Passed. |
| Conventions and PR template | Passed locally and in hosted CI. |
| Python syntax | Passed. |
| Python test registration | Passed: 230 scripts are registered or workflow-run. |
Merge preview against current main |
Clean. |
| Hosted Ubuntu relinker and ASan | Passed. |
| Hosted Windows relinker | In progress when checked. |
| Hosted macOS relinker jobs | Queued when checked. |
| Product Linux/Windows and specialized jobs | Skipped. |
| Test-quality heuristic | Could not run because its helper is absent from the isolated PR snapshot. |
| Local relinker build or execution | Not run; this test requires a built relinker binary. |
Unedited precheck output
All conventions checks passed.
230 test script(s), each run by an add_test or a workflow
Negative control
| Assertion | Change to code under test | Outcome |
|---|---|---|
| The two macOS TLS-invalid fixtures are rejected and produce no artifacts | Make the converter accept either invalid fixture | Could not be exercised: the new macOS loop does not inspect result, artifact, or output, so acceptance would not fail this test. |
Findings
Blocking
In test_guest_none_relocations.py, both cases yielded by invalid_cases_macos() call convert() and discard the returned values. Unlike the Linux and Windows loops, there is no assertion that the converter returns 2 with the expected diagnostic or that it leaves no artifact/output. Thus these cases do not test the rejection they are intended to cover.
Please assert the expected status and diagnostic, and assert that neither artifact nor output is created, as the adjacent Linux and Windows invalid-case loops do.
Non-blocking
None.
Sources
- macOS invalid-case loop and the neighboring asserted Windows loop
- The
convert()helper returns results and artifact paths without asserting conversion success - Exact-head hosted checks
Not verified
I did not build or execute the relinker locally, and the macOS relinker jobs were queued at the last check snapshot. No claim is made that these macOS fixtures passed or failed conversion.
To get this to a good state
Assert the macOS cases’ return code, diagnostic, and absence of output artifacts. I will rerun the same precheck and review the new head.
|
Reviewed at Please assert exit code 2, the expected diagnostic, and absence of both the converted module and output executable, as the adjacent Linux and Windows loops do. Include the target system and case name in assertion context. Then rerun the complete Python test and confirm that a control which wrongly accepts a macOS invalid fixture makes it fail. The previous invalid-case names are retained; the missing checks are confined to the new macOS loop. |
|
Fixed at 7a61f80: the macOS invalid-case loop now asserts the same things as the Linux and Windows loops (exit code 2, the expected diagnostic, and absence of the converted module and output executable), so the two macOS TLS cases can no longer pass if the image is accepted. |
7a61f80 to
68d33bd
Compare
|
Updated at 68d33bd. The macOS invalid-case loop now asserts exit code 2, the expected diagnostic, and that neither the converted module nor the output executable exists, matching the Linux and Windows loops, and its assertion context includes the target system and case name. Verification: |
|
Thanks for the review. The macOS invalid-case loop now asserts the same rejection properties as the Linux and Windows loops: |
|
Current-main follow-up for head Main also contains merged #15526, which splits the fixture into The current-main fixture comparison ran on Windows with WinLibs GCC 15.2; I did not run a refreshed full #14079 CI composition. Details of the same comparison are recorded on #10351. AI-assisted: yes. |
68d33bd to
3ad8c73
Compare
|
Thanks for the follow-up. Rebased onto current main and refreshed the three expectations: AI-assisted: yes |
|
Checked the new invalid cases against the harness (exit code 2, exact diagnostic, no converted module or output on Linux, macOS and Windows), the refreshed DT_REL/RELA/UNUSED expectations and the green relinker jobs on every platform; merged. |
What
Consolidates the
guest_none_relocationsinvalid-case coverage into a single change. The harness gains the helpers andinvalid_casesentries for guest dynamic-tag rejection (DT_REL, DT_RELA, DT_STRTAB, DT_SYMBOLIC, DT_TEXTREL, DT_UNUSED, DT_RPATH), duplicate dynamic segments and SCE dynamic data, unterminated terminators, unsupported dynamic flags, guestPT_TLSsegment validation (alignment, memsz, outside file/load), guest lifecycle-array validation, guest preinit-without-init, and guest symbol/relocation rejection, including the Windows/macOS-only TLS cases. Replaces the parallel per-case pull requests on this file with one, so the file has a single open pull request.Tested
Debian 12 x86-64, Python 3.12, relinker-only Release
-DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON:ctest -R guest_none_relocations— pass.ctest -j 8— 540/540.python3 tools/check_conventions.py --base upstream/main— passed.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)