Repository navigation
test(tools): register SELF cases independently - #14558
Conversation
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
BunnyFl
left a comment
There was a problem hiding this comment.
Windows 11, WinLibs GCC 15.2.0 r7, relinker-only Release configure (-DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON). Head 69750ca merged into upstream/main 93dbd0f:
ctest -N -R '^unwrap_self'lists 80 tests.unittestloads exactly 80 cases fromtest_unwrap_self.py, so every case is registered once.- The kept
unwrap_selfname carriesUNWRAP_SELF_TEST=<class>.<case>. ctest -R '^unwrap_self' -j8: 80/80 passed in 7.2 s.
This follows the existing per-case pattern used for trace_digest, and nothing later in the root CMakeLists.txt sets ENVIRONMENT on unwrap_self. With CMAKE_CONFIGURE_DEPENDS on the test source, cases added by other open PRs (#14534 adds one) are picked up at the next configure.
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — no blocking findings
The CMake change registers each test_unwrap_self.py case as its own CTest entry: the first case is attached to the existing unwrap_self test through an environment variable, and the others are added with add_test. The Python main reads that variable and runs only the named case. I read the CMake logic and the test's main change. The hosted Windows run included only one of the new per-case entries, so most of them did not run hosted.
What I ran
- Head
69750caed2ef28e48d6bd2455d1737302b03a1b9; basef2d874c1fdccf781c0fa5d86619bb5dd8894b958. 2 files, +22/-1:CMakeLists.txt(per-case registration) andtools/tests/test_unwrap_self.py(environment-selectedmain). - Read the full CMake block and the
mainchange. The baseunwrap_selftest is defined atCMakeLists.txtline 257 at this head, so the first case has a test to attach to. - Read the test classes and cases (
class UnwrapTestand itstest_*methods, lines 94-193 at head). - Read-only checks from the trusted main checkout (precheck not run: this session does not run code from PR heads; no Python or CMake configure was run here): diff check, conventions and the stack check passed.
- Hosted: the windows CTest run passed 2594 of 2594 cases; the log shows
unwrap_self_UnwrapTest_test_phdr_mapping_other_load_bytes_preserves_input_and_output(Test #314). The other per-case entries do not appear in that log. - No MarcZX10 review at this head.
Results
| Check | Result | Note |
|---|---|---|
| Diff check | passed | |
| Conventions and PR template | passed | conventions check from the main checkout |
| Mergeability and dependencies | passed | depends on none; the stack check passed |
| CMake per-case registration logic | passed | read; the first case reuses unwrap_self, the others use add_test |
main selects the named case |
passed | read; unittest.main(argv=[...]) with the environment value |
| Per-case entries in the hosted run | partial | one of the new entries appears in the Windows log; the rest do not |
| Build and runtime tests | not run | no local build; no Python or CMake run here |
| Negative controls | not run | no local build |
| Hosted CI | passed | windows passed (2594 cases); linux and relinkers skipped |
Read-only check output
Precheck was not run: this session does not run code from PR heads. Hosted: gh api repos/boykopovar/AnyPS5/commits/69750caed2ef28e48d6bd2455d1737302b03a1b9/check-runs.
Negative control
Not run here. A negative control needs a build and running the Python tests, which is not done for code from PR heads in this session.
Findings
Blocking
None.
Non-blocking
- The hosted Windows run did not include most of the new per-case entries. Please say where the cases are run (for example the Linux job, or a local run) so the coverage can be checked.
Sources
- CMake change: https://github.com/boykopovar/AnyPS5/blob/69750caed2ef28e48d6bd2455d1737302b03a1b9/CMakeLists.txt
- Test
main: https://github.com/boykopovar/AnyPS5/blob/69750caed2ef28e48d6bd2455d1737302b03a1b9/tools/tests/test_unwrap_self.py - Base test definition:
Line 257 in 69750ca
Not verified
- The per-case entries were not run; the CMake configure and the Python tests were not run here.
- Whether the Linux job runs all per-case entries was not checked.
To get this to a good state
- Nothing blocking. A note of where the per-case entries run would close the coverage gap.
|
Correction to my review 5732216632 (COMMENTED, head 69750ca). The Hosted CI row says "linux and relinkers skipped". That is wrong. Every relinker job at this head completed with success before the review was posted: relinker (ubuntu-latest, g++), relinker (windows-latest, g++), relinker (macos-latest, clang++), relinker (macos-latest, clang++, x86_64) and relinker-asan. Linux and register-context were skipped, and the main windows job passed. The other rows are unchanged. |
What
Register each existing SELF unittest case as a separate CTest with the existing 30-second timeout. The aggregate suite exceeds that timeout on Windows even though its individual cases finish within it. Preserve the existing
unwrap_selftest name for one explicitly selected case, and track the test source as a configure dependency so new cases are registered automatically.Tested
Head
69750caed2ef28e48d6bd2455d1737302b03a1b9, refreshed against mainf2d874c1fdccf781c0fa5d86619bb5dd8894b958.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)