Repository navigation
test(tools): run each conventions_wrapped_declarations case as its own test - #15259
Merged
oneandonlydean merged 2 commits intoNov 4, 2026
Conversation
…n test Each case builds a git fixture and starts check_conventions.py, so the two per-class tests from boykopovar#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>
check_test_registration.py counts a script as run only when an add_test argument names its path, so passing it through a variable left test_check_conventions_wrapped_declarations.py unregistered and failed test_script_registration in every relinker job. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
12 tasks done
12 tasks done
BunnyFl
approved these changes
Nov 4, 2026
BunnyFl
left a comment
BunnyFl
left a comment
Contributor
There was a problem hiding this comment.
@JJenkx approved at a45263e026.
What I ran: this head merged onto main in the real tree (Windows 11, CMake configure of the full project).
ctest -N -R '^conventions_wrapped_declarations'lists 17 tests.- That matches the
test_methods in the file. Parsing it withastgives 10 inWrappedDeclarationTests, 7 inAbiAliasTestsand none inWrappedDeclarationFixture, which theTests\(regex rightly skips. ctest -R '^conventions_wrapped_declarations' -j4: 17 of 17 pass.- With
-V, each of the 17 printsRan 1 test: one case per registration.
Read in the CMake: the first case keeps the old conventions_wrapped_declarations name, and every other case becomes <Class>.<method>, which unittest.main() accepts. The class comes from the nearest class ...Tests( line above each method (CMakeLists.txt, the CMAKE_MATCH_1 of the class branch).
Optional, not blocking: the ^ def test_ pattern only sees methods indented exactly four spaces. A future test_ method written another way, for example inside a nested helper class, would be left unregistered without any error. A configure-time count could catch that, comparing file(STRINGS ... REGEX "def test_") with the number registered.
12 tasks done
Contributor
✅ AMD GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (10, not counted against this PR)Passed on retry (2, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validated but not compared (66, not in the main baseline's validation pass)Validation IDs already present on main (8 IDs in 783 tests)Skipped (62)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Contributor
|
@JJenkx Additional consumer-side validation for Using current main
Both discover all17 AST-listed methods exactly once; verbose output confirms The original two class groups also pass locally on both plain main and unchanged #7085, so I did not reproduce the hosted timeout. This validates your registrations and our composition, not the full686-test run or hosted timing. No native/relinker build or GPU run was performed for these Python-only checks; our published branch is unchanged. AI-assisted: yes (Codex). |
12 tasks done
12 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Register every
test_method intools/tests/test_check_conventions_wrapped_declarations.pyas its own CTest test, with a 20 s limit. The first case keeps theconventions_wrapped_declarationsname; each other case isconventions_wrapped_declarations_<Class>_<method>. No test logic changes.unittest.main()already acceptsClass.methodon the command line.Why
Each case creates a git fixture with two commits and then starts
check_conventions.pyas a subprocess. #14412 split the test by class, but on the Windows runner the two class tests still exceed 30 s. Recent mainBuildruns where thewindowsjob failed only on these two timeouts:9090972b7e(run 41518923690):conventions_wrapped_declarationsandconventions_wrapped_declarations_AbiAliasTests, both***Timeoutat 30.05 and 30.09 sf7d225d8e3(run 41514145453): the same two testsLocally the classes take 6.3 s (10 cases) and 5.9 s (7 cases), so the runner is about 5× slower under the parallel CTest load. One case at a time keeps each test far below its limit. This uses the same approach as #15085 (
annotate_module).Tested
Windows 11 (build 22621), AMD Ryzen 9 5950X, Python 3.15, CMake 4. The new block was configured in a minimal CMake project against this file:
ctest -Nlists 17 tests (10 + 7), the first namedconventions_wrapped_declarations.ctest -V: 17 of 17 pass, each output showsRan 1 test, the slowest takes 1.9 s, and the total is 14.5 s.check_conventions.py --base origin/mainpasses.check_test_registration.py --scripts(thetest_script_registrationtest in the relinker jobs) passes: 250 test scripts, each run by an add_test or a workflow. The first head,b422a25929, failed it because the script path was only passed through a variable;a45263e026names the path in theadd_testcall.Independent Windows validation by BunnyFl of exact head
a45263e026merged onto main: a full-project CMake configuration discovers all seventeen cases, matching tenWrappedDeclarationTestsand sevenAbiAliasTestsmethods;ctest -R '^conventions_wrapped_declarations' -j4passes 17/17, and verbose output confirms each invocation runs one test. The reviewer reports Windows 11; compiler version and host CPU are unspecified. The four-space discovery-pattern caveat is an optional follow-up; this attribution leaves the approved source head unchanged.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)🤖 Generated with Claude Code