Skip to content

fix(tools): select tests for build artifact arguments - #10619

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
xTRACKGOD:fix/select-tests-argument-inputs
Oct 29, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
xTRACKGOD:fix/select-tests-argument-inputs

Conversation

@xTRACKGOD

Copy link
Copy Markdown
Contributor

What

Selective CI can skip a compiled test that receives another build artifact as an argument. dump records the consumer executable's inputs but omits the argument executable's inputs. In the configured Linux relinker graph, changing ElfReader.cpp selects elf_reader_bounds while omitting all six compiled relinker CLI consumers, including rela_bounds.

Include source/header closures for recognized Ninja outputs passed as raw arguments or -DNAME=... values. Store those inputs in the existing per-test arguments field, keeping the graph schema compatible. Source-file arguments retain their handling; missing paths and directories are filtered before querying Ninja.

Tested

Windows x86-64, Python 3.12.14, CMake 4.3.1, Ninja 1.13.2, WinLibs GCC 15.2.0; baseline main 8ec6397b3762fa23f90a6973d223b12553e420a5:

  • New source/header regression fails on unchanged main because both the raw-path and definition-argument consumer tests are omitted. The fixed complete selector suite passes: python -m unittest tools.tests.test_select_tests, 34/34 tests, 72.444 seconds on this host. Existing Windows junction fixtures ran outside sandbox restrictions.
  • Independent native CMake/Ninja reproduction: the original project passes 3/3 tests. After changing the provider's shared header, main's selected run passes while full CTest fails the omitted compiled consumer. This change selects that consumer and catches the same failure; an unrelated source still selects only its own test. Explanation output names the shared header as the selection reason.
  • Actual repository graph comparison: a Linux x86-64 relinker-only CMake configuration contains 244 tests. The unchanged selector omits all six compiled relinker CLI consumers for ElfReader.cpp; the fixed graph selects all six alongside elf_reader_bounds. This check used configuration data only, not native Linux execution.
  • Regressions cover source/header dependencies, raw and -D artifact arguments, ordinary source arguments, unrelated tests, nonexistent output paths and directory arguments. Independent source review found no additional issue.
  • All 175 Python test scripts are registered. Conventions, PR title/body validation and git diff --check origin/main...HEAD pass.
  • tools/pr_overlap.py --branch fix/select-tests-argument-inputs checked 1,249 open PRs and reported no overlap, with no uncompleted comparisons. The unmodified repository checker used a local adapter for public GitHub pagination because gh is unavailable. No dependency.

No title, console hardware, GPU or full system-library build was tested. No application performance claim is made.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head a5c5d9908a0c547fd553889dc4d840f0018d9e60. The selector now resolves recognized Ninja outputs passed as raw test arguments or -D values, then adds those outputs’ source/header closures to the corresponding test inputs; ordinary source-file arguments remain tracked and missing paths/directories are excluded. The regression exercises both raw and definition arguments and ensures provider source/header edits select their consumers. On this exact head, python3 -m unittest tools.tests.test_select_tests passed (34 tests, 1 skipped), python3 tools/check_conventions.py --base upstream/main passed, and git diff --check upstream/main...HEAD passed. Hosted eligibility, intake, conventions and labels pass; Linux/Windows relinker and ASan are still running, macOS jobs queued. I did not run the full project build.

@roboterspinne roboterspinne left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head a5c5d99 merged onto upstream/main 66de8e7 (2026-10-24); the merge is clean, check_conventions.py passes and pr_overlap.py reports no overlap.

Environment: Windows 11 Home 10.0.26200, Python 3.13.9 (Anaconda), CMake 4.3.1, Ninja 1.13.2, WinLibs GCC 15.2.0; TEMP pointed outside AppData so the junction fixture can be created.

Results

  • python -m unittest tools.tests.test_select_tests on the merged tree: 34 of 34 pass in 22 s. Red/green: the new test against main's select_tests.py fails both subcases (alpha/tool.c, alpha/tool.h: the argument and definition consumers are missing from the selection); on the branch it passes in 4 s.
  • Real graph: on a relinker-only Windows configuration (configured, not compiled, so header deps are absent) dump lists 244 tests, 36 with a build executable and 208 Python-driven. The branch adds four closure targets (relinker.exe, guest_module_args_host.prx, the two windows_stdio_host DLLs) and grows the arguments of 140 tests, every one of them Python-driven, which select() always picks ("no build executable"). For a change to ElfReader.cpp or LinuxElfPatcher.cpp both selectors pick the same 219 tests, so on Windows the selection does not change; the graph file grows from 102 KB to 800 KB.
  • The six compiled consumers the body names (to_intel_linux_stubs, rela_bounds, syscall_scanner, guest_intel_trampolines_execution, dynamic_table_bounds, reloc_table_bounds) are registered only under if(CMAKE_SYSTEM_NAME STREQUAL "Linux" AND CMAKE_SYSTEM_PROCESSOR MATCHES "x86_64|AMD64") (core/relinker/CMakeLists.txt:868-897), and add_test_executable links nothing from the relinker (CMakeLists.txt:10-17; RelaBoundsTests.cpp includes only ElfFixture.hpp and RelinkerProcess.hpp). So on Linux their closures on main hold only their own sources, and the body's "omitted for ElfReader.cpp" is consistent with the build graph; I could not run the Linux graph itself.
  • The new code: paths() already strips -D...=, build.output keeps only paths inside the build dir, build.outputs filters them through ninja -t targets all, the closures are computed for those targets like for test executables, and each test's inputs gain the closure of its argument outputs; missing paths and directories drop out at the outputs filter.

Nits

  • select_tests.py:211: each consuming test gets the whole closure of its argument output copied into arguments, although that closure already sits in closures (the output is added to targets at line 187). On the Windows tree relinker.exe's 88 files are repeated in 139 tests and the graph grows from 102 KB to 800 KB with no change to the selection; on a built full tree with header deps and the $<TARGET_FILE:libc> consumers it will be larger. Storing the output names in arguments and resolving them through closures in select() would keep the graph flat, at the cost of a small schema change; your call, the cache can hold 800 KB.
  • test_select_tests.py:217: the "directory argument" of alpha_not_built is ${CMAKE_BINARY_DIR} itself, which Build.output() already maps to None (the root is not inside itself), so it never reaches the new build.outputs() filter; only missing.txt exercises it. A subdirectory such as ${CMAKE_BINARY_DIR}/alpha would make the directory coverage the body claims real.
  • The new test checks only the selected names; the body's "explanation output names the shared header as the selection reason" has no assertion in the repository. Since the reason comes from the same files & scope intersection that picks the test, assertIn("input: alpha/tool.h", reasons["alpha_tool_argument"]) would be optional hardening.

Notes, nothing to change here: a side effect the body does not mention: because 140 tests now list the relinker closure under arguments, those files are in select()'s known set, and the directory fallback no longer fires for them. For core/relinker/main.cpp the Windows graph selects 208 tests instead of 240 (the 32 dropped compiled tests were picked only as directory neighbours and none compiles main.cpp); eleven relinker-only files behave like this, always with fewer tests, never more, so a precision gain. Artifacts passed as directories (-DMODULES=$<TARGET_FILE_DIR:libc>) or through ENVIRONMENT_MODIFICATION PATH entries stay untracked, as on main. #9556 edits the deps parsing at the top of dump() and its test file; the hunks are separated by unchanged lines and apply on top of this branch, but whichever lands second should rerun the selector suite. The body's 72 s suite time is the author's host; here it is 22 s.

Not checked: the Linux graph and the Linux-only consumers themselves, header deps (the tree was not compiled), macOS, the body's native CMake/Ninja reproduction and its pr_overlap adapter.

I found no blocking issue: the new inputs are computed from the same Ninja graph as the test executables' closures, the filter drops directories and missing paths, plan() already builds argument outputs for selected tests, the schema stays loadable for cached graphs, and the counter-check holds. For disclosure: I have open PRs (#10667 on tools/tests/test_suggest_patches.py, and relinker PRs #10658, #10646, #10607, #10596, #10564, #10403, #10402, #10401, #10391, #10390); none touches these files.

Written with AI assistance (Claude Code). I ran the selector tests and the graph comparison above on my Windows 11 machine.

"add_test(NAME alpha_tool_direct COMMAND alpha_tool)\n"
"add_test(NAME alpha_tool_argument COMMAND alpha_tests $<TARGET_FILE:alpha_tool>)\n"
"add_test(NAME alpha_tool_definition COMMAND alpha_tests -DTOOL=$<TARGET_FILE:alpha_tool>)\n"
"add_test(NAME alpha_not_built COMMAND alpha_tests ${CMAKE_BINARY_DIR}/missing.txt ${CMAKE_BINARY_DIR})\n",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the directory argument is the build root, which Build.output() already maps to None (inside() needs root plus separator), so it never reaches the new build.outputs() filter; only missing.txt exercises it. ${CMAKE_BINARY_DIR}/alpha would make the directory case the body claims real.

Comment thread tools/select_tests.py
arguments = sorted({build.repo(a) for a in paths((test.get("command") or [])[1:])} - {None})
tests[test["name"]] = {"registered": sorted(registered - {None}), "arguments": arguments,
inputs = {build.repo(a) for a in paths((test.get("command") or [])[1:])} - {None}
inputs.update(path for target in arguments[test["name"]] & outputs for path in closures[target])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this copies the whole closure of each argument output into every consuming test's arguments, although closures already holds it (the output joins targets at line 187). On a relinker-only Windows tree relinker.exe's 88 files repeat in 139 tests and the graph grows from 102 KB to 800 KB without changing the selection; a built full tree with header deps and the $<TARGET_FILE:libc> consumers will be larger. Storing the output names here and resolving them through closures in select() would keep the graph flat, at the cost of a small schema change; your call.

@THEMANnt THEMANnt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed a5c5d9908a0c against pinned base 6cd36e651724. No actionable findings in this focused review.

Source review

dump() now includes source/header dependencies of recognized Ninja outputs passed as absolute raw or -D arguments. Those dependencies enter the existing per-test arguments field, so the selector can include compiled consumers when their argument artifact's inputs change. Ordinary source arguments remain supported, and unrecognized output paths are filtered before dependency traversal. Existing assertions are preserved; there are no local review amendments.

Validation

Controller-recorded execution of tools/tests/test_select_tests.py on Windows with Python 3.11.9 reports:

  • Baseline: 33 passed, 0 skipped; 33 total.
  • Pristine PR and candidate: each 34 passed, 0 skipped; 34 total.
  • Candidate tests against old code: two assertion failures in the new source/header subtests, both showing the omitted raw and -D consumers; no setup errors.

The regression uses real CMake/Ninja graphs and selector execution. Local conventions passed. GitHub-reported CI shows successful relinker matrix and ASan jobs plus eligibility, conventions, intake, and labels; windows, linux, register-context, and write-watch were skipped.

Scope

Validation covers the Python tool and miniature build fixtures. Full application behavior and the author's additional native reproduction and Linux graph claims were not independently tested.

@oneandonlydean oneandonlydean self-assigned this Oct 25, 2026
@fogydes

fogydes commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

OK

  1. [read-only] The new argument handling records the build inputs for test arguments that resolve to Ninja outputs, including -DNAME=...; I found no defect in the changed code or regression cases. Test outcomes in the PR description are [author-reported]; I did not run the Python suite in this clone.
  2. [verified] python tools/check_conventions.py --base 8ec6397b3762fa23f90a6973d223b12553e420a5 passed in a scratch worktree.

Freshness envelope: head a5c5d9908a0c547fd553889dc4d840f0018d9e60; merge-base 8ec6397b3762fa23f90a6973d223b12553e420a5; origin/main is 4,338 commits ahead of the merge-base; git merge-tree --write-tree origin/main pull/10619 exited 0 (clean).

Cross-PR ledger: #11668 (fix(tools): reject dangling test graph targets) and #12219 (fix(tools): report an unreadable select.tests in select_tests ctest as a usage error) touch the same files. git merge-tree --write-tree pull/10619 pull/11668 and the corresponding command with pull/12219 both exited 0, so each pair merges cleanly.

@oneandonlydean
oneandonlydean merged commit e0da13c into boykopovar:main Oct 29, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants