Repository navigation
Conversation
…er siblings Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The new 120-second CTest limit conflicts with the repository's explicit 30-second per-test rule, which says to split or shrink tests that approach the limit instead of raising their timeout.
What I ran
Reviewed the only changed file at head 96ece96e85f509e8aa979a909022357b99236cac against base 4f41f4c375f5abb487dbd184fbc150f31994d762; the exact-head merge preview is clean. Environment: Debian 12, Linux x86_64, Python 3.11.2, Git 2.39.5; I did not compile or build. I inspected the timeout change, the repository's timing rule, the PR's reported test timings and current hosted check state.
git diff --check 4f41f4c375f5abb487dbd184fbc150f31994d762...96ece96e85f509e8aa979a909022357b99236cac
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 15441
python3 tools/pr_test_quality.py --root /tmp/anyps5-15441-quality.HbZ5PO --json core/libs/prx/libSceAgcDriver/tests/cmake/agc_driver_recorder_tests.cmake
gh pr checks 15441 --repo boykopovar/AnyPS5The PR description reports three sanitizer runs on base d1ae628bc2 at 23.07, 24.44 and 24.09 seconds and a prior nightly run that timed out at 30.25 seconds. Those are author-reported results, not runs I performed.
Results
| Check | Result | Note |
|---|---|---|
| Diff, conventions, and merge preview | passed | No formatting issue; exact-base preview is clean. |
| Test-quality heuristic | passed | No heuristic findings for the CMake change. |
| Repository timing rule | failed | The change raises the test limit to 120 seconds despite the 30-second policy. |
| Hosted policy checks | passed | Conventions, intake, labels, eligibility, portability, and report passed. |
| Hosted Windows job | not run | In progress at the final status refresh; I did not run it. |
| Hosted Linux and specialized jobs | not applicable | GitHub skipped these jobs. |
| Local build and CTest | not run | No build or test run was needed to inspect this timeout-only edit. |
Commands and unedited outputs
Precheck table:
| 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 4f41f4c375f5abb487dbd184fbc150f31994d762...96ece96e85f509e8aa979a909022357b99236cac |
| conventions_and_pr_template | passed | /usr/bin/python3 tools/check_conventions.py --base 4f41f4c375f5abb487dbd184fbc150f31994d762 --head 96ece96e85f509e8aa979a909022357b99236cac --title test(agc): give the driver recorder test the same timeout as its slower siblings --body /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15441-96ece96e85f5/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. |
| related_python_unit_tests | not_applicable | No changed tools/ module has an existing sibling tools/tests/test_<module>.py suite. |
| python_test_registration | not_applicable | No changed test_*.py files. |
| live_dependency_and_mergeability_check | passed | /usr/bin/python3 /marc/AnyPS5/tools/pr_stack_check.py 15441 --repo boykopovar/AnyPS5 --output-dir /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15441-96ece96e85f5/stack |
| build_dependent_cpp_tests | not_applicable | No C/C++ implementation or test files changed. |
| test_quality_heuristics | failed | /usr/bin/python3 /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15441-96ece96e85f5/head-96ece96e85f5-dd7c0f6a/tools/pr_test_quality.py --root /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15441-96ece96e85f5/head-96ece96e85f5-dd7c0f6a --json core/libs/prx/libSceAgcDriver/tests/cmake/agc_driver_recorder_tests.cmake |
| negative_controls | not_applicable | No changed Python test module in tools/ can be run as a no-build base-source control. |
The precheck heuristic command could not start because its isolated PR snapshot lacks the workspace-only script. The direct helper run against the exact changed file returned:
{
"findings": [],
"negative_control_candidates": [],
"limits": [
"Heuristics can miss weak assertions and can flag valid tests; inspect every finding.",
"A test that passes against base is not by itself proof that the PR test is inadequate.",
"Negative controls are only candidates until the test actually runs against base and fails by assertion rather than import/setup error."
]
}The exact-head stack report says the merge preview is clean and no dependency is declared. Hosted status at review: Windows was in progress; conventions, intake, labels, eligibility, library-portability and report passed; Linux and specialized jobs were skipped.
Negative control
| Assertion | Change to code under test | Did the new test fail? |
|---|---|---|
agc_driver_recorder should complete within the repository's 30-second CI budget. |
No mutation or local execution; I reviewed the PR's reported runs and the timeout declaration. | Not run by me; the PR reports one nightly CI timeout at 30.25 seconds. |
Findings
Blocking
The test's timeout is raised from 30 to 120 seconds at the reviewed head. Repository policy says each test finishes within 30 seconds on CI and explicitly directs contributors to split or shrink a test that gets close, rather than raising its timeout. The PR reports a 30.25-second timeout and recent runs around 24 seconds, so the reported evidence shows the test is already at or over the project's budget. Please reduce or split the test so it stays within 30 seconds under the normal CI load instead of extending its timeout.
Non-blocking
None.
Sources
- Repository's 30-second test budget and direction to split or shrink slow tests
- The 120-second timeout at the reviewed head
Not verified
I did not run the test under ASan, reproduce the nightly timeout, build, or run CTest locally. The three recent timings and the nightly timeout are reported by the PR author.
To get this to a good state
Reduce or split the test to meet the repository's 30-second budget, then rerun the focused sanitizer test under the relevant CI conditions. If the head changes, I will rerun the diff, conventions, precheck and merge preview on the new head.
XairRrR
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The timeout only happens under the sanitizer, but the new 120 s limit applies to every build, including normal CI, where the test takes about 6 s. The repository already has a way to raise a limit only in ASan builds, and this change should use it.
What I ran
Head 96ece96e85f509e8aa979a909022357b99236cac, main 4f41f4c375f5abb487dbd184fbc150f31994d762, merged tree 2d43fd7829323171be33a3fa74929222f0a23b00. Machine: Debian 13 VM with 32 vCPU (Xeon Gold 6136), GCC 14.2.0, Release, lavapipe (Mesa 26.1.6). No sanitizer build.
git diff --check 4f41f4c375f5abb487dbd184fbc150f31994d762...96ece96e85f509e8aa979a909022357b99236cac
python3 tools/check_conventions.py --base 4f41f4c375f5abb487dbd184fbc150f31994d762
ctest --test-dir build -R '^agc_driver_recorder$' --output-on-failure # three times, one after the otherThe script also built the merged tree and ran the selected tests and the Khronos validation comparison.
Results
| Check | Result |
|---|---|
Merge into main |
clean |
git diff --check |
passed |
check_conventions.py |
passed |
Build and libs |
passed |
| Selected tests (1403) | 1377 passed, 24 expected skips, 2 failures that also fail on main (unwrap_self_..._destination_directory_group, vector_constant_factor_tininess); no new failures |
agc_driver_recorder, Release, 3 runs |
passed in 5.63 s, 8.05 s and 5.58 s |
| Validation layer | loaded in 209 of 210 tests and missing in none (the script's list of tests without the layer is empty). Its one "new" VUID (VUID-vkCmdDispatch-groupCountX-00386) comes from agc_driver_native_depth_uniform_planes, which did not run on the baseline. This PR only changes a CMake timeout in another fragment, so the VUID is not from this PR. |
ctest output
1/1 Test #1971: agc_driver_recorder .............. Passed 5.63 sec
1/1 Test #1971: agc_driver_recorder .............. Passed 8.05 sec
1/1 Test #1971: agc_driver_recorder .............. Passed 5.58 sec
Negative control
None. The PR changes only a test timeout and no production code or test logic, so there is nothing to revert.
Findings
Blocking
The limit is raised for every build, but the timeout only happens under the sanitizer (core/libs/prx/libSceAgcDriver/tests/cmake/agc_driver_recorder_tests.cmake:380 in the merged tree; L354 at head).
- Evidence that the problem is sanitizer-only: the PR's own timings and the failing nightly run all come from the ASan+UBSan build. In a Release build here the test takes 5.6 to 8 s, well below 30 s.
- Why this conflicts with the rules: CONTRIBUTING.md L19 says to split or shrink a test that gets close to 30 s instead of raising its timeout. With an unconditional 120 s limit, the normal Linux and Windows CI jobs would no longer stop this test if it slowed down about fifteenfold.
- Where I differ from MarcZX10's review: I agree the change breaks that rule. I don't think the test needs splitting, though, because it is nowhere near 30 s outside the sanitizer.
- Existing precedent: the repository already solves this exact case in agc_pad_status_overlay_tests.cmake L86-L88:
agc_performance_overlaykeepsTIMEOUT 30and gets 120 s only insideif(ANYPS5_ENABLE_ASAN). That test also has an entry in.github/sanitizer-known-failures.txt, line 4. - Suggested fix: keep
TIMEOUT 30on the existing line and add the same ASan-only override:
if(ANYPS5_ENABLE_ASAN)
set_tests_properties(agc_driver_recorder PROPERTIES TIMEOUT 120)
endif()Non-blocking
- The slowdown is not explained. By the description's own numbers, the sanitizer runtime roughly tripled between
926f7dd492(about 7 s) andd1ae628bc2(about 24 s), and it was not bisected. A higher limit hides that regression rather than explaining it. Please bisect it, or open an issue for it, so the slowdown is followed up. - The title says "siblings" (plural), but only one sibling has 120 s. In this fragment, only
agc_storage_write_backuses 120 s; the other larger limits are 60 s. This is minor: please reword the title or description.
Not verified
- I did not build with
-DANYPS5_ENABLE_ASAN=ONor UBSan, so I did not reproduce the ~24 s sanitizer timings or the 30.25 s nightly timeout. Those are the author's numbers. - I did not check whether the Release runtime also grew between
926f7dd492andmain. - I did not check the overlap table against other PRs.
To get this to a good state
- Restore
TIMEOUT 30and raise the limit only insideif(ANYPS5_ENABLE_ASAN), asagc_performance_overlaydoes. - Retitle the PR, or reword the description, to say the change is for sanitizer builds.
- Optionally, bisect the sanitizer slowdown between
926f7dd492andd1ae628bc2, or record it somewhere it will be followed up.
✅ AMD GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (5, not counted against this PR)Validation IDs already present on main (4 IDs in 446 tests)Skipped (7)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. |
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks to both reviewers. The new head 95f01f3 keeps AI-assisted (Claude Code). |
XairRrR
left a comment
There was a problem hiding this comment.
Re-checked — approved
The new commit 95f01f3601 keeps TIMEOUT 30 for agc_driver_recorder and raises it to 120 s only when ANYPS5_ENABLE_ASAN is on, as I suggested. That resolves my blocking finding.
What changed since my last review
- New commit
95f01f3601("raise the driver recorder test limit only in ASan builds"). The first commit,96ece96e85, is unchanged. Together the PR now leaves the existingTIMEOUT 30line alone and adds anif(ANYPS5_ENABLE_ASAN)block that setsTIMEOUT 120. - The title and description were rewritten to say the change is for sanitizer builds. The author's 12:21 comment reports the effective timeout from
ctest --show-only=json-v1with ASan (120) and without it (30), and says the slowdown is still not bisected.
What I ran
Head 95f01f3601506c913ddb9ea37229a237faf0a111, main 4f41f4c375f5abb487dbd184fbc150f31994d762, merged tree 29b759b7f3f11487608ff73660459afb5b4ab983. Machine: Debian 13 VM, 32 vCPU (Xeon Gold 6136), GCC 14.2.0, Release, lavapipe (Mesa 26.1.6). This build has ANYPS5_ENABLE_ASAN=OFF.
git diff --check 4f41f4c375f5abb487dbd184fbc150f31994d762...95f01f3601506c913ddb9ea37229a237faf0a111
python3 tools/check_conventions.py --base 4f41f4c375f5abb487dbd184fbc150f31994d762
ctest --test-dir build -R '^agc_driver_recorder$' --show-only=json-v1 # TIMEOUT property
grep -n ANYPS5_ENABLE_ASAN build/CMakeCache.txt
grep -rn 'ANYPS5_ENABLE_ASAN\|fsanitize' .github/workflows/*.yml
git status --porcelainThe script also built the merged tree, ran the selected tests and ran the Khronos validation comparison.
| Check | Result |
|---|---|
Merge into main |
clean |
git diff --check |
passed |
check_conventions.py |
passed ("All conventions checks passed.") |
Build and libs |
passed |
| Selected tests (1382) | 1357 passed and 23 were expected skips. Two tests failed, and both also fail on main (unwrap_self_..._destination_directory_group, vector_constant_factor_tininess). No new failures and no timeouts. |
| Validation layer | loaded in 207 of 208 tests and missing in none; 0 tests with new VUIDs |
Effective TIMEOUT of agc_driver_recorder, non-ASan build |
30.0 |
| Sanitizer workflow sets the guard | yes: .github/workflows/sanitizers.yml:20 passes -DANYPS5_ENABLE_ASAN=ON |
Previous findings
- Blocking: the limit was raised for every build. Resolved. At
core/libs/prx/libSceAgcDriver/tests/cmake/agc_driver_recorder_tests.cmake:380-383in the merged tree, the lineTIMEOUT 30is unchanged and the override to 120 sits only insideif(ANYPS5_ENABLE_ASAN). This is the same pattern asagc_performance_overlayin agc_pad_status_overlay_tests.cmake L86-L88. In this non-ASan build, ctest reportsTIMEOUT30.0. The value of 120 under ASan is the author's report; I did not configure an ASan build. - Non-blocking: the sanitizer slowdown is not explained. Still open. The description and the author's comment say plainly that it was not bisected. That is acceptable for a timeout change, but it would still be good to follow up on it.
- Non-blocking: the title said "siblings" (plural). Resolved. The PR title and the new commit now describe an ASan-only limit. The first commit's subject still says "siblings", which only matters if the PR is merged without squashing.
Negative control
None. The PR changes only a CTest property and no production code or test logic, so there is nothing to revert.
New findings
Blocking
None.
Non-blocking
agc_performance_overlayhas an entry in.github/sanitizer-known-failures.txtthat explains its ASan timeout.agc_driver_recorderhas none, and with the 120 s limit it does not need one. No action is needed.
Not verified
- I did not configure or run an ASan/UBSan build, so I checked neither the effective 120 s limit under ASan nor the sanitizer runtimes of about 24 s. Both are author-reported.
- I did not reproduce the 30.25 s timeout of the nightly run, and I did not bisect the slowdown.
- I did not check the overlap table.
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (4 IDs in 446 tests)Skipped (4)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
✅ AMD GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (5, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validation IDs already present on main (4 IDs in 446 tests)Skipped (7)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. |
What
The nightly sanitizer run 41573880531 (job address-undefined, head f6232fc) stopped
agc_driver_recorderat its limit:Timeout 30.25 sec. The run of 2026-11-02 on 926f7dd passed it in 6.88 s.agc_driver_recorderhasTIMEOUT 30. Under the sanitizer build the test took about three times as long ond1ae628bc2as on 926f7dd in the measurement below, which leaves it close to the limit. In a build without the sanitizer it stays at 30 s. This keepsTIMEOUT 30and sets 120 only whenANYPS5_ENABLE_ASANis on, the wayagc_performance_overlayis set in agc_pad_status_overlay_tests.cmake L86-L88. Not bisected; which change slowed the test under the sanitizer is not known, and a higher limit in that build can also hide a later slowdown.Overlap
All 1678 open PRs were listed on 2026-11-03 at 10:21 UTC and their changed files read until 11:16 UTC, against
main4f41f4c375; the merge tests below ran with this change applied on thatmain. The file of this change iscore/libs/prx/libSceAgcDriver/tests/cmake/agc_driver_recorder_tests.cmake. Twenty-seven other open PRs change it; each was merged with and without this change (git merge-tree):main)No PR differs between the two columns, so this change creates no conflict.
Tested
Ubuntu 24.04.5 on WSL2 (kernel 6.18.54.1), gcc 13.3.0, Release, 2026-11-03 10:41 to 11:08 UTC, on
main4f41f4c375with this change. The effectiveTIMEOUTofagc_driver_recorderfromctest --show-only=json-v1:-DANYPS5_ENABLE_ASAN=ON(the flags of the nightly sanitizer workflow,-g1 -fsanitize=undefined -fno-sanitize=vptr): 120.agc_performance_overlaythere: 120.agc_performance_overlaythere: 30.Runtime of
ctest -R '^agc_driver_recorder$', one test at a time, three runs:Earlier sanitizer runs on the commits before (07:31 to 07:34 UTC, a pull request file sweep on the same machine): 926f7dd took 9.52 s, 6.72 s and 6.99 s;
d1ae628bc2took 23.07 s, 24.44 s and 24.09 s. The timeout of the nightly run was not reproduced, and this change was not run under a controlled load.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)AI-assisted (Claude Code).
🤖 Generated with Claude Code