Repository navigation
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
amd-bfilipov
left a comment
There was a problem hiding this comment.
Looks good, thanks.
marbre
left a comment
There was a problem hiding this comment.
------------------------------ Captured log call -------------------------------
ERROR test_artifact_structure:test_artifact_structure.py:452 MISSING package coverage: amd-dbgapi [test] (amd-dbgapi_test_generic.tar.zst)
=========================== short test summary info ============================
FAILED tests/test_artifact_structure.py::TestArtifactStructure::test_artifacts_covered_by_packages - Failed: Found 1 artifact component(s) not covered by package.json:
amd-dbgapi [test] (amd-dbgapi_test_generic.tar.zst)
========================= 1 failed, 2 passed in 10.46s =========================
Can you please address this @lumachad?
Will do. Thanks for spotting that. |
40e8201 to
7770bec
Compare
7770bec to
5d19d13
Compare
5d19d13 to
8d84836
Compare
Split rocr-debug-agent CI configuration into CPU and GPU test jobs, following the pattern established for amd-dbgapi in #8943. - rocr-debug-agent-cpu: CPU runner, placeholder test script - rocr-debug-agent-gpu: GPU runner, placeholder test script Both jobs use echo placeholders that always pass until the test wrapper script is updated with --cpu-tests and --gpu-tests flags. Added rocr-debug-agent to TEST_LABEL_GROUPS so test:rocr-debug-agent label triggers both variants. JIRA ID: AIROCGDB-635
Split rocr-debug-agent CI configuration into CPU and GPU test jobs, following the pattern established for amd-dbgapi in #8943. - rocr-debug-agent-cpu: CPU runner, placeholder test script - rocr-debug-agent-gpu: GPU runner, placeholder test script Both jobs use echo placeholders that always pass until the test wrapper script is updated with --cpu-tests and --gpu-tests flags. Added rocr-debug-agent to TEST_LABEL_GROUPS so test:rocr-debug-agent label triggers both variants. JIRA ID: AIROCGDB-635
0bbf069 to
0d65574
Compare
0d65574 to
79a2f53
Compare
79a2f53 to
e790c61
Compare
e790c61 to
48b1085
Compare
PR Pundit review(Testing PR Pundit Code Review agent. Not all comments may be relevant. This is a one-time automated pass: please address what is useful, no need to request a re-review. A quick reply per point (useful or not, and why) helps us calibrate the agent. Reach out to @peymanr for suggestions and feedback.)
1. 🟠 Should fix: Echo-only amd-dbgapi placeholder jobs scheduled by default, wasting GPU runners: this PR added ungated matrix entries
Who is affected: Every Linux and Windows CI test run with debug-tools artifacts schedules two no-op jobs per GPU family. Checked against the code by an independent pass. Related past reviews: #2231 on 2. 🟠 Should fix: amd-dbgapi Windows jobs fetch nonexistent artifact patterns: this PR added Windows to Linux-only
|
48b1085 to
c52ed31
Compare
Fair enough. I'm leaving the test enablement in CI for a follow-up. |
c52ed31 to
b9209a5
Compare
|
I'd like to merge this reduced scope PR later today. Do let me know if you have objections. |
| amd-llvm | ||
| CMAKE_ARGS | ||
| -DTHE_ROCK_BUILD=ON | ||
| "-DENABLE_TESTS=${THEROCK_BUILD_TESTING}" |
There was a problem hiding this comment.
Thanks for the changes @lumachad, can ENABLE_TESTS stay off until an upstream unit test actually exercises amd-dbgapi? For my understanding: the new binary dbgapi_smoke_test only links GTest::gtest_main and GTest::gmock. It does not call amd-dbgapi. Does the follow-up add that unit test, or does it only run this smoke binary?
There was a problem hiding this comment.
can ENABLE_TESTS stay off until an upstream unit test actually exercises amd-dbgapi?
I'd rather leave the ENABLE_TESTS gated on THEROCK_BUILD_TESTING, as that means we always go through the infra to build these and install the files. That way we'll always test this path when we bump rocm-systems, even though we don't have a complete set of unit tests or invoke them.
For my understanding: the new binary dbgapi_smoke_test only links GTest::gtest_main and GTest::gmock. It does not call amd-dbgapi. Does the follow-up add that unit test, or does it only run this smoke binary?
The follow-up will start running these CPU-based smoke tests first, then the set will be expanded to do real CPU-based tests. Then we will eventually have GPU-based ones.
Enablement of the unit tests is taking place in parallel, and will be done in incremental steps.
Does that answer your questions?
There was a problem hiding this comment.
Thank you . This explains why ENABLE_TESTS stays tied to THEROCK_BUILD_TESTING.
And reconfirms The smoke binary, the missing amd-dbgapi unit test, and the later GPU tests are expected in follow-up
There was a problem hiding this comment.
@lumachad at this time dbgapi_smoke_test is included in packaging, but not getting tested with CI, right?
Would you have a link to dbgapi_smoke_test test results to make sure it runs fine?
There was a problem hiding this comment.
@dileepr1 That's correct. I did the validation locally on Linux. The Windows runners had some hiccups that prevented running it. I think I can put up a second temporary commit to at least confirm the smoke test runs fine on both Windows and Linux. Would that be acceptable?
Once we see it working, I can drop the temporary patch and go with just building it but not running it in CI.
There was a problem hiding this comment.
That would be great, thanks!
There was a problem hiding this comment.
Pushed a temporary commit now.
There was a problem hiding this comment.
@dileepr1 @arvindcheru Here's one of the Windows smoke test jobs running: https://github.com/ROCm/TheRock/actions/runs/42214365465/job/127909265685?pr=8943
And the Linux one: https://github.com/ROCm/TheRock/actions/runs/42214365465/job/127910206450?pr=8943
rocdbgapi now builds a unit test suite through ENABLE_TESTS and ENABLE_UNIT_TESTS, installing the test binaries under a dedicated install component. Wire THEROCK_BUILD_TESTING through to ENABLE_TESTS so the suite is built whenever testing is enabled, add therock-googletest as a build dependency when that's the case, and fold the test binaries into the amd-dbgapi artifact as a new test component so they get staged and distributed like everything else. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Temporary patch to try the dbgapi-cpu test job in CI. Adds a test matrix entry on the rocgdb CPU container and a script that runs each GoogleTest binary installed under tests/rocdbgapi. Not meant to merge with the test build PR. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
c505c71 to
b9e5b8d
Compare
Summary
rocdbgapi now builds a unit test suite upstream, gated by
ENABLE_TESTSandENABLE_UNIT_TESTS, and installs the test binaries under a dedicatedtestinstall component.THEROCK_BUILD_TESTINGis wired through toENABLE_TESTSso the suite is built whenever testing is enabled, andtherock-googletestis added as a build dependency only in that case.The test binaries are folded into the existing
amd-dbgapiartifact as a newtestcomponent rather than a separate artifact, matching the pattern already used by rocgdb. They are excluded from thedevcomponent so they only ship once.build_tools/packaging/linux/package.jsonwas updated so the new component is covered by theamdrocm-debugger-testpackage.The committed consumer graph was updated with the new
amd-dbgapitotherock-googletestbuild dependency edge.This PR only builds and packages the tests. Running them in CI is left for a follow-up.
ISSUE ID: AIROCGDB-634
🤖 Generated with Claude Code