Repository navigation
test(tools): split conventions_wrapped_declarations by test class to stay under 30 s on Windows - #14412
Conversation
…stay under 30 s on Windows
|
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.
|
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — no blocking findings
Splitting the 17-test module into two test classes and registering the second class separately is a correct test-only refactor: the 17 tests run unchanged and pass at head, and the CMake class discovery matches the file.
What I ran
- Exact head
c186f7b2ccc36f75c392ecfe0587f667f6c0b7a9, baseba627802e2210187d417deb64c8f2fed8e33011f. - Environment: Linux,
/usr/bin/python33.11.2, no build. - Precheck:
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14412 --output-dir /dev/shm/reviewer-run/precheck/14412 - Head
tools/extracted withgit archiveto/dev/shm/reviewer-run/src2/14412full, then, from that tree:
python3 -m unittest tools.tests.test_check_conventions_wrapped_declarations(no-I)
python3 tools/tests/test_check_conventions_wrapped_declarations.py AbiAliasTests
python3 tools/tests/test_check_conventions_wrapped_declarations.py WrappedDeclarationTests - CMake class discovery run in script mode (
cmake -P), which generates no build outputs, using the samefile(STRINGS ... REGEX)asCMakeLists.txt.
Results
Precheck report, checks table, unedited (/dev/shm/reviewer-run/precheck/14412/report.md):
| Check | Status | Command / reason |
|---|---|---|
| build_and_compile | not_applicable | The precheck deliberately does not configure, build, or compile AnyPS5. |
| diff_check | passed | git diff --check ba627802e2210187d417deb64c8f2fed8e33011f...c186f7b2ccc36f75c392ecfe0587f667f6c0b7a9 |
| conventions_and_pr_template | passed | /usr/bin/python3 tools/check_conventions.py --base ba627802e2210187d417deb64c8f2fed8e33011f --head c186f7b2ccc36f75c392ecfe0587f667f6c0b7a9 --title test(tools): split conventions_wrapped_declarations by test class to stay under 30 s on Windows --body /dev/shm/reviewer-run/precheck/14412/pr-body.md |
| changed_export_abi_audit | not_applicable | No changed source or public header under core/libs/prx. |
| python_syntax | not_applicable | No changed Python implementation files. |
| unit:tools/tests/test_check_conventions_wrapped_declarations.py | passed | /usr/bin/python3 -m unittest tools.tests.test_check_conventions_wrapped_declarations |
| python_test_registration | passed | /usr/bin/python3 tools/check_test_registration.py --scripts |
| live_dependency_and_mergeability_check | passed | /usr/bin/python3 /marc/AnyPS5/tools/pr_stack_check.py 14412 --repo boykopovar/AnyPS5 --output-dir /dev/shm/reviewer-run/precheck/14412/stack |
| build_dependent_cpp_tests | not_applicable | No C/C++ implementation or test files changed. |
| test_quality_heuristics | failed | /usr/bin/python3 /dev/shm/reviewer-run/precheck/14412/head-c186f7b2ccc3-9cae4046/tools/pr_test_quality.py --root /dev/shm/reviewer-run/precheck/14412/head-c186f7b2ccc3-9cae4046 --json CMakeLists.txt tools/tests/test_check_conventions_wrapped_declarations.py |
| negative_controls | failed | A pass means every candidate failed by assertion on base; a failure means at least one did not discriminate or could not run. See per-test results. |
Manual results:
manual unittest output (head c186f7b)
Ran 17 tests in 3.550s
OK
Ran 7 tests in 1.617s
OK
Ran 10 tests in 1.603s
OK
cmake -P class discovery output
-- base test keeps class WrappedDeclarationTests
-- would add conventions_wrapped_declarations_AbiAliasTests
Test-quality helper: one heuristic finding at line 123 (test_alias_undefinition_redefinition_and_cycles_do_not_create_exports has no recognized assertion). That test calls self.assertNotRegex, so the finding is a false positive.
Negative control
Not a discriminating control for a refactor. The precheck run on base reported did_not_fail_on_base because the base module runs the same 17 tests. The discriminating check is the CTest registration (ctest -N), which needs a configure step and was not run.
Findings
Blocking: none.
Non-blocking:
- Timing margin: the author's quiet-host numbers put the two classes at about 14-15 s each against the 30 s bound. The margin under the CI Windows runner is not measured here.
Sources
- Registration at head:
Lines 268 to 283 in c186f7b
Not verified
ctest -Nregistration and the Windows CI timing were not run (configure needs a build).- Hosted: windows in progress and macOS clang queued at review time.
…n test Each case builds a git fixture and starts check_conventions.py, so the two per-class tests from #14412 still passed 30 s on the Windows runner and timed out on main (9090972, f7d225d). Register every test_ method as its own test with a 20 s limit; the first keeps the conventions_wrapped_declarations name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
conventions_wrapped_declarationsruns all 17 tests oftools/tests/test_check_conventions_wrapped_declarations.pyas one ctest test, and 15 of them callcheck()20 times in all, each building a two-commit git repository and runningpython -I check_conventions.py --base HEAD~1 --head HEADas a subprocess (check(), lines 16-46 on 26fa121; line numbers below refer to it). On the Windows job ofmainBuild runs it took 33.38 s (run 41288908159), 33.23 s (41288882563), 32.62 s (41292426874) and 35.03 s (41288949548), and the slow-test step (.github/workflows/build.yml:731-737) warned in each run, for exampleconventions_wrapped_declarations took 33.4 s on Windows; tests must finish in 30 s (CONTRIBUTING.md)in run 41288908159 (see Tested). CONTRIBUTING.md:19 requires each test to finish in 30 s on the CI runners and to be split or shrunk when it gets close instead of raising its timeout. The repository already splits Python test files by class:check_main_syncandselect_testsread the class names withfile(STRINGS ... REGEX "^class [A-Za-z0-9_]*Tests\\(")and register one test per class (CMakeLists.txt:306-318and319-330).tools/check_conventions.pyis unchanged andcheck()still runs the checker as an isolated-Isubprocess with theGITHUB_*variables removed.check()moves unchanged intoWrappedDeclarationFixture(unittest.TestCase), whose name does not match the class regex and which has no tests of its own, asCheckMainSyncFixtureandSelectFixturedo. The ten duplicate-scan tests stay inWrappedDeclarationTests(10 checker runs); the seven ABI-alias tests (lines 69-118: local alias, chained alias, undefinition and cycles,cpp_text, aliased import, silent stub, unique aliased definition; 10 checker runs, while the undefinition andcpp_texttests call the checker's functions in-process) move unchanged intoAbiAliasTests. CMake registersconventions_wrapped_declarationswith the literal script path andWrappedDeclarationTestson argv, socheck_test_registration.py --scriptsstill finds the script, and addsconventions_wrapped_declarations_<Class>for every other*Testsclass with the loop ofcheck_main_sync, each withTIMEOUT 30. Each test now runs about half the checker subprocesses of the old one.python tools/tests/test_check_conventions_wrapped_declarations.pywith no argv still runs all 17.pr_overlap.py --branchfinds no shared lines with the open pull requests it can compare and cannot compare feat(aot): discuss native ARM64 compiler architecture and integration #1835, feat(aot): consolidate native ARM64 compiler, runtime and consumers #5021, feat(agc): look up table images at run time through a device image table on the GPU heap (APS5_GPU_PATH) #7642, perf(agc): GPU descriptor heap on VK_EXT_descriptor_heap (APS5_GPU_HEAP) #9508, fix(tools): merge only the pull requests that share files or exports with the branch in pr_overlap #9539, feat(tools): export progress history as CSV #12766, feat(libSceIcu): implement property and case exports from ICU tables #13578, feat(progress): count stubs no measured use needs as unobserved #13801 and docs(debt): separate entries with blank lines and drop derived export totals #14212, which also changeCMakeLists.txtbut do not merge withmain. Checked by hand: a trial merge of this branch with each of them conflicts in exactly the files thatmainwith it already conflicts in, so this branch adds no conflict, andCMakeLists.txtmerges cleanly except for fix(tools): merge only the pull requests that share files or exports with the branch in pr_overlap #9539, which conflicts in it onmainas well.Tested
Windows 11, WinLibs GCC 15.2.0, relinker-only Release build (
cmake -S . -B build-relinker -G Ninja -DCMAKE_BUILD_TYPE=Release -DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON), Python 3.13.9 (Anaconda), Git for Windows; synthetic fixtures only, no title or homebrew.main, Build workflow,windowsjob (ctest --parallel 4):Test #49: conventions_wrapped_declarations ... Passedin 33.38 s (run 41288908159), 33.23 s (41288882563), 32.62 s (41292426874) and 35.03 s (41288949548), each with the warningconventions_wrapped_declarations took 33.4 s(33.2, 32.6, 35.0 s)on Windows; tests must finish in 30 s (CONTRIBUTING.md), read withgh run view <run> --log.ctest --test-dir build-relinker -N -R conventions_wrappedlists one test (conventions_wrapped_declarations), andpython tools/tests/test_check_conventions_wrapped_declarations.py AbiAliasTestsfails withAttributeError: module '__main__' has no attribute 'AbiAliasTests'. On this branch it listsconventions_wrapped_declarations(... test_check_conventions_wrapped_declarations.py WrappedDeclarationTests) andconventions_wrapped_declarations_AbiAliasTests(... AbiAliasTests), both withTIMEOUT 30.Ran 17 tests ... OK, withWrappedDeclarationTests10 and withAbiAliasTests7, all OK.ctest -R conventions_wrapped_declarationsthen took 15.26 s and 14.07 s, and 14.55 s and 13.64 s, for the two tests. With other builds loading the host to 100 %, the whole file took 222.3 s and 104.1 s and the two classes 92.3 s and 79.3 s, and 54.7 s and 44.4 s; under that load the old test took 179.93 s and 113.03 s in ctest on 26fa121, and both new tests andcheck_main_sync,check_main_sync_CheckMainSyncHistoryTestsandcheck_main_sync_CheckMainSyncEncodingTests, which have the sameTIMEOUT 30, timed out.ctest --test-dir build-relinker -j 6on this branch with other builds running: 553 tests, 542 passed,long_pathsskipped. Failed:select_testsand its partsselect_tests_SelectGraphStateTests,select_tests_SelectBuildTestsandselect_tests_SelectArgumentTestswith the Windows symlink fixture'sFileExistsError, as onmainon this machine; timed out under the load:annotate_module,conventions_wrapped_declarations,conventions_wrapped_declarations_AbiAliasTests,check_main_sync,check_main_sync_CheckMainSyncHistoryTestsandcheck_main_sync_CheckMainSyncEncodingTests. Rerun alone once the host was quiet, all nineannotate_module,conventions_wrapped_declarations*andcheck_main_sync*tests passed (13.70, 15.34, 15.23 and 3.03-9.71 s).test_registrationandtest_script_registrationpassed.python tools/check_conventions.py --base 26fa1212b8 --head HEADwith this title and description 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)