Skip to content

fix(ci): reject tests that were not run before skip exemptions - #11671

Merged
adriadam10 merged 2 commits into
boykopovar:mainfrom
StoneDC:fix/check-skips-notrun
Oct 28, 2026
Merged

adriadam10 merged 2 commits into
boykopovar:mainfrom
StoneDC:fix/check-skips-notrun

Conversation

@StoneDC

@StoneDC StoneDC commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

What

A missing CTest executable is reported as status="notrun"; the coverage checker must reject it before the Windows agc* or device-skip exemptions. CTest also uses notrun for tests intentionally skipped through SKIP_RETURN_CODE and SKIP_REGULAR_EXPRESSION, so rejecting every notrun case incorrectly rejects legitimate skips.

Reject notrun unless its <skipped message> is a recognized CTest skip marker (SKIP_RETURN_CODE=<number> or SKIP_REGULAR_EXPRESSION_MATCHED). A recognized marker permits the existing job-specific reason policy to run; it does not exempt an unexpected reason. Disabled tests remain rejected even if they carry a skip marker, and the existing disabled diagnostic is retained.

The real-CTest regressions cover a missing executable, accepted exit-77 and regex skips, and unexpected reasons through both skip mechanisms. Additional fixtures reject unmarked/malformed notrun and disabled cases. Every policy check runs for Linux and Windows where applicable. No GPU/display access is required for these classifier fixtures.

The original 34 test method bodies are preserved. Keep the original aggregate registration and place the six new cases in a separate test file and CTest group. The new group has a 30-second timeout. This also keeps #11062's adjacent sanitizer-report registration independent. No prerequisite PR is required. The #9161 projection below covers only the shared checker and tests, not that PR's native implementation.

Tested

Exact tested head: 29e43ebd5b25304cd272bcfab2a60bde96df5047, rebased onto upstream main 09f9a63f6b85aec9f36ce5d0d46f41c37cbd86fd.

Windows: Python 3.14.3, MinGW GCC 15.2.0, CMake/CTest 4.3.1, Ninja. WSL Ubuntu: Python 3.14.4, GCC 15.2.0, CMake/CTest 4.2.3, Ninja.

Configured with -G Ninja -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=ON -DANYPS5_RELINKER_ONLY=ON; ran ctest --test-dir <build> -R '^check_skips' --parallel 4 -V --output-junit <report>.

  • windows: 2/2 CTest groups, 40 unittest cases, zero failures/skips; slowest group 13.72 s.
  • linux: 2/2 CTest groups, 40 unittest cases, zero failures/skips; slowest group 4.56 s.

Both platforms actually invoke CTest to generate JUnit reports. Accepted SKIP_RETURN_CODE=77 / regex skips produce status="notrun" and pass the checker with exit 0 for both jobs. Real CTest skips with an unexpected flaky reason still fail the checker with exit 1. The missing-executable fixture makes CTest fail and the checker rejects it with exit 2 for both jobs, including the Windows agc_missing name. Disabled and malformed/unmarked cases remain rejected. These are real CTest classification tests, not actual hardware capability skips.

Negative control on both hosts: the two accepted-skip regressions run against the prior PR's unchanged production checker and fail all four job assertions (2 != 0). The final checker passes those regressions while retaining the missing-executable rejection. No missing-module/import error is used as regression evidence.

Native relinker-only builds and relinker --help pass on Windows and Linux. Current-main conventions, whitespace, the test-script registration audit (198 scripts), XML skip-policy audits and Python 3.9 grammar parsing pass. Grammar parsing is not execution on an older Python runtime.

Tool-only integration with #9161 head d086e569050ed0d3a68d20444f8eae5a39cb10fd: the shared checker and test file merge cleanly onto this exact head using common ancestor 74acfad65b3f0fd4eab653f3a3989d298fba1157. projection-windows: 2/2 CTest groups, 41 unittest cases, zero failures/skips, slowest 12.11 s. projection-linux: 2/2 CTest groups, 41 unittest cases, zero failures/skips, slowest 5.06 s. This validates only those two shared tool/test files and their registration; no whole-PR native build, symlink system-call behavior or GPU execution is claimed for #9161.

The repository overlap CLI was rerun for this final branch. The earlier adjacent CMake conflict with #11062 was removed by retaining the original check_skips registration; per-file three-way merges at #11062 head 2d04e57 and #7264 head a49d9d1 are clean. Shared CMake changes in legacy branches were also inspected: six per-file merges are clean; #8588 and #11366 have the same conflict blocks against main alone. These do not implement this classifier fix, and their whole-PR mergeability is not claimed.

Not run: native macOS, an older Python runtime, full system-library/product builds, game/PS5 execution or GPU validation. New hosted CI is separate from these local results; skipped product jobs are not counted as passes. No performance improvement is claimed.

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 (host tools only)
  • AI-assisted: yes

@MarcZX10

Copy link
Copy Markdown
Contributor

Independent local validation at 94ed3e084019c49e150654625afc900cea6cb225: I ran python3 -m unittest tools.tests.test_check_skips from a fresh archive of this exact head; 34 tests passed. check_conventions.py against current main and git diff --check also pass. Hosted Ubuntu relinker and ASan pass; Windows and macOS relinker jobs are still pending. Product jobs are skipped by path policy. I have not submitted a review while the hosted matrix is incomplete.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

Thanks, catching a test whose program is missing is worth doing. One problem: CTest writes status="notrun" for every skipped test, including ones skipped through SKIP_RETURN_CODE (<skipped message="SKIP_RETURN_CODE=77"/>), so with this change every accepted skip (the Windows agc_* tests, devices without a display or Vulkan) makes the check fail on both CI jobs. Could you reject notrun only when the skipped message is not a CTest skip status, such as Unable to find executable? A second real-ctest case with a SKIP_RETURN_CODE test that prints an accepted reason, and still passes, would pin that.

@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.

I reproduced a CI regression in the new status="notrun" check.

[P1] Allow CTest's intentional SKIP_RETURN_CODE results. I created a one-test CTestTestfile.cmake test that exits 77 with SKIP_RETURN_CODE 77 and writes skipped, no display. CTest exits 0 and generates JUnit with <testcase ... status="notrun"><skipped message="SKIP_RETURN_CODE=77"/>.... Running this PR's check_skips.py rejects that report with “is notrun and was not run” before applying the existing accepted-skip policy. The same CTest status is used by the expected Windows agc_* skips and Linux device/display capability skips, so this will make the skip-check jobs fail on legitimate skips.

Please distinguish a missing/unlaunchable test (which must fail) from a test that CTest intentionally skipped through SKIP_RETURN_CODE or SKIP_REGULAR_EXPRESSION_MATCHED. Add a real CTest regression that confirms an accepted exit-77 skip passes, while the missing-executable case remains rejected. The current unit fixture only covers the latter and therefore misses the conflict.

Validation: reproduced against exact head 94ed3e084019c49e150654625afc900cea6cb225 with the installed CTest: CTest generated status="notrun" and the PR checker exited 2 for the accepted skip.

@StoneDC
StoneDC force-pushed the fix/check-skips-notrun branch 2 times, most recently from 08e80ff to 5acfa3a Compare October 27, 2026 03:42
@StoneDC

StoneDC commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor Author

@oneandonlydean @MarcZX10 Fixed in 29e43ebd5b25304cd272bcfab2a60bde96df5047, rebased onto main 09f9a63f6b85aec9f36ce5d0d46f41c37cbd86fd.

notrun now reaches the existing skip-reason policy only when <skipped message> is a recognized CTest skip marker. A marker alone does not excuse an unexpected reason. Missing/unlaunchable tests and disabled tests remain rejected before device or Windows agc* exemptions.

The new tests invoke real CTest for both SKIP_RETURN_CODE=77 and SKIP_REGULAR_EXPRESSION: accepted display/Vulkan reasons pass both job policies, while unexpected reasons still fail. The original real missing-executable regression remains, and disabled/unmarked/malformed cases stay covered. Before the fix, the accepted-skip tests reproduced four assertion failures on each host; they now pass.

Final-head Windows and WSL validation: 2/2 related CTest groups, 40 unittest cases each, zero skips/failures. All original 34 method bodies were retained, and the six new cases live in a separate file/group with a 30-second timeout. Both native relinker-only builds/help, current-main conventions, whitespace, registration and XML skip audits pass.

The shared-tool projection with #9161 also passes 2/2 groups and 41 unittest cases on each host, with zero skips; this is not validation of its native system-call changes. The PR description has been rewritten around the corrected classification and exact-head evidence. Native macOS, older Python runtime, full product and actual GPU capability checks were not run locally. New hosted CI has been triggered; please recheck this head.

AI-assisted: yes.

@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 5acfa3ab620d18c4ddb1d32294f02e7219f6dee8. Comment: the intentional-skip regression reported on the prior head is fixed in my local control; hosted validation for this new head is still pending.

Windows/Python 3.11.9 and CTest 4.3.3: all 40 skip-checker tests passed. A separate real CTest exit-77 fixture produces status="notrun" with an intentional skip marker: the previous head rejects it, while this head accepts it under the Windows AGC policy. The submitted cases also check accepted and unexpected return-code and regex skips, disabled entries and missing executables. The baseline accepts a missing executable under the Windows exemption, so the missing-executable control demonstrates the intended correction.

The latest Build run has not started its jobs at review time; the successful run on the preceding revision does not validate this new test registration. Please confirm the new registered suites in hosted CI. No GPU execution or full product build is claimed.

@StoneDC
StoneDC force-pushed the fix/check-skips-notrun branch from 5acfa3a to 29e43eb Compare October 27, 2026 03:47
@github-actions

github-actions Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

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.

Pull request Shared files not compared
PR 674 CMakeLists.txt
PR 1835 CMakeLists.txt
PR 3786 CMakeLists.txt
PR 5021 CMakeLists.txt
PR 9508 CMakeLists.txt
PR 9539 CMakeLists.txt
PR 10897 CMakeLists.txt

@StoneDC

StoneDC commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

@MarcZX10 @THEMANnt @oneandonlydean The final head is 29e43ebd5b25304cd272bcfab2a60bde96df5047. Its Build run 40446138078 has completed successfully; the run's head SHA matches this final commit, rather than the preceding revision.

I checked the job logs: both check_skips and the new check_skips_ctest group actually ran and passed in every relinker matrix job:

Job check_skips check_skips_ctest
Ubuntu Passed Passed
Windows Passed Passed
macOS native Passed Passed
macOS x86_64 Passed Passed

The relinker-ASan job and conventions check are also green. Full product, register-context and write-watch jobs were skipped by policy; these results do not claim full product or GPU validation.

Could you please re-review this final head? If the intentional-skip correction and real CTest regressions resolve the P1 finding, please update the changes-requested review on 94ed3e0840 accordingly. Thank you.

AI-assisted: yes.

@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 29e43ebd5b25304cd272bcfab2a60bde96df5047. Approve: the intentional CTest skip regression is fixed, and the new test group is registered beside the existing checker without changing its timeout.

Windows/Python 3.11.9 and CTest 4.3.3: all 40 skip-checker tests passed. My real exit-77 fixture is accepted by this head under the Windows AGC policy; the original PR head rejected it, while the old baseline accepted a missing executable. The submitted tests also cover regex skips, unexpected reasons, malformed markers and disabled or missing tests. The checker and test files are unchanged from the previously tested head; the CMake registration moved.

Hosted Windows, Linux and both macOS relinker jobs ran both registered checker suites on merges containing this head. Product jobs were skipped. These are host-side classifier tests; no GPU or full product build is claimed.

@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.

Re-reviewed final head 29e43ebd5b25304cd272bcfab2a60bde96df5047. The notrun classification now accepts only recognized CTest skip markers, then still applies the existing job/reason policy; disabled, unmarked/malformed and missing-executable cases remain rejected. The new real-CTest regressions cover accepted and unexpected SKIP_RETURN_CODE and regex skips. I ran the two checker suites from a fresh archive of this exact head: 40 tests passed, including the generated JUnit cases. check_conventions.py and git diff --check pass against refreshed main. Hosted Ubuntu, Windows, both macOS relinker jobs and ASan are green; product jobs are skipped by path policy. This is host-side skip classification only; no GPU behavior is claimed. The earlier blocking finding is resolved.

@adriadam10 adriadam10 self-assigned this Oct 27, 2026
@github-actions github-actions Bot added the build label Oct 27, 2026
@adriadam10
adriadam10 merged commit 566f13b into boykopovar:main Oct 28, 2026
13 of 14 checks passed
@adriadam10

Copy link
Copy Markdown
Collaborator

Re-reviewed against current main 957a22d51. The only main change near this hunk is the sanitizer_report registration just below check_skips; the merge with this head is clean.

  • notrun now reaches the reason policy only for recognized CTest skip markers (SKIP_RETURN_CODE=<n>, SKIP_REGULAR_EXPRESSION_MATCHED); missing, unmarked/malformed and disabled cases are still rejected.
  • The real-CTest cases cover accepted and unexpected reasons for both skip mechanisms.
  • Dean's point is addressed; MarcZX10 and THEMANnt approved.

Checked the notrun classification, the real-CTest skip cases and the merge with main 957a22d51; merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants