Skip to content

COMP: Label wrapped tests correctly and report the limited-API setting - #6849

Merged
dzenanz merged 2 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:comp-itk-wide-build-ux
Sep 6, 2026
Merged

COMP: Label wrapped tests correctly and report the limited-API setting#6849
dzenanz merged 2 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:comp-itk-wide-build-ux

Conversation

@hjmjohnson

Copy link
Copy Markdown
Member

Two build-UX fixes that apply to every wrapped module, split out of the ITKVtkGlue-specific work in #6848. Both come from independent Windows validation of #6715.

  • Wrapped tests now carry their own module's label. ctest -L ITKVtkGlue went from 8 tests to 11 here.
  • The resolved ITK_USE_PYTHON_LIMITED_API value is printed, since it auto-enables by interpreter version.
1. Wrapped tests were labelled with the wrong module

itk_python_add_test delegates to itk_add_test, which labels a test with ${itk-module}. That variable is not set in the Wrapping/Modules/<Name>/test scope, so every Python test inherited whichever module happened to be configured last — in this build, ITKFFTImageFilterInit:

Labels: ITKFFTImageFilterInit Python

That was true of every Python test in the build, not just one module's.

The visible symptom is that the two obvious selectors disagree while both returning the same count, which hides the problem:

ctest -L ITKVtkGlue                        -> 8 tests
ctest -R "VtkGlue|ImageToVTK|VTKImageTo"   -> 8 tests   (a different 8)

Union is 11. Fixed by setting itk-module before the wrapping test directory is added, so itk_add_test labels correctly for all modules.

Measured before and after, same configuration:

ctest -L ITKVtkGlue
before 8
after 11
2. Print the resolved limited-API setting

ITK_USE_PYTHON_LIMITED_API auto-enables from the interpreter version in ITKSetPython3Vars.cmake, so "I did not pass it" does not mean OFF. Nothing reported the resolved value, which made it easy to believe a build was testing the opposite mode from the one it was in.

-- Python wrapping: limited API (abi3) ON [auto-selected from Python 3.13.9]
-- Python wrapping: limited API (abi3) ON [requested]
What was dropped from this PR, and why

The same Windows report described a third issue: with ITK_BUILD_DEFAULT_MODULES=OFF, -DModule_ITKTestKernel=ON without the :BOOL type leaves the entry INTERNAL=OFF, the dependent C++ tests are never created, and ctest reports 100% pass on what remains.

I wrote a configure-time warning for it and then removed it, because I could not reproduce the underlying condition on current main. In a build with ITK_BUILD_DEFAULT_MODULES=OFF and Module_ITKVtkGlue=ON, the C++ tests register whether or not Module_ITKTestKernel:BOOL=ON is passed. A first attempt at the warning also fired on every dependency-pulled module, which legitimately builds no tests.

Rather than ship a diagnostic for a state I cannot demonstrate, it is left out. If someone can produce a configuration where the tests silently vanish, that belongs in its own issue with the exact flags.

Verification

macOS arm64, VTK 9.6.2, Python 3.13.9:

  • Configure succeeds; pre-commit run --all-files exits 0.
  • Label change measured against an unpatched build of the same source as a control: 8 before, 11 after.
  • Status line verified in both branches of the auto-selection logic.

@hjmjohnson
hjmjohnson marked this pull request as ready for review September 6, 2026 15:43
@github-actions github-actions Bot added type:Compiler Compiler support or related warnings type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots area:Python wrapping Python bindings for a class labels Sep 6, 2026
@greptile-apps

greptile-apps Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This change improves Python wrapping test labeling and limited-API status reporting.

T-Rex validation blocked

CMake is unavailable in the environment, so the two-configure cache-origin check could not run. The limited-API message can misidentify an automatically selected value as user-requested, and the wrapping comment must meet the repository's one-line comment requirement before merging.

Confidence Score: 4/5

Not safe to merge until the limited-API status correctly preserves whether its value was automatic and the repository's comment requirement is satisfied.

The cached configuration path has a blocking correctness concern. The comment also violates an explicit repository requirement.

Files Needing Attention: CMake/ITKSetPython3Vars.cmake and Wrapping/macro_files/itk_end_wrap_module.cmake.

T-Rex T-Rex Logs

What T-Rex did

  • I reviewed the limited-API cache-origin logic in CMake/ITKSetPython3Vars.cmake, focusing on how the NOT DEFINED vs DEFINED branches determine the 'requested' status.
  • I authored a minimal two-configure reproduction that reuses a single build directory and captures the limited-API status line.
  • I attempted to run the two-configure reproduction, but the runs halted before configuration because /bin/sh: cmake: not found, so the reconfiguration could not be exercised.
  • I ran trex-artifacts/limited-api-cache-origin-validation.sh to create a fresh minimal CMake project, configure the same build directory twice, and filter the limited-API status line, exiting safely when CMake is unavailable.
  • I identified the blocker as the missing cmake binary in the environment, consistent with both proofs.

View all artifacts

T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "COMP: Report the resolved Python limited..." | Re-trigger Greptile

Comment thread CMake/ITKSetPython3Vars.cmake
Comment thread Wrapping/macro_files/itk_end_wrap_module.cmake Outdated
itk_python_add_test reaches itk_add_test, which labels a test with
${itk-module}. That variable is not set in the wrapping test scope, so
every Python test inherited whichever module was configured last.

The effect was that "ctest -L ITKVtkGlue" and a name regex selected
different sets, each of which happened to contain eight tests.
ITK_USE_PYTHON_LIMITED_API auto-enables from the interpreter version, so
not passing it does not mean OFF. Print the value and where it came from.
@hjmjohnson
hjmjohnson force-pushed the comp-itk-wide-build-ux branch from 2d44148 to c7c9d43 Compare September 6, 2026 16:07
@dzenanz
dzenanz merged commit 30fa369 into InsightSoftwareConsortium:main Sep 6, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Python wrapping Python bindings for a class type:Compiler Compiler support or related warnings type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants