Repository navigation
debug-tools: Build and package amd-dbgapi test suite #8943
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
lumachad
wants to merge
2
commits into
main
Choose a base branch
from
users/lumachad/dbgapi-tests
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
46 changes: 46 additions & 0 deletions
46
build_tools/github_actions/test_executable_scripts/test_dbgapi.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| # Copyright Advanced Micro Devices, Inc. | ||
| # SPDX-License-Identifier: MIT | ||
|
|
||
| """ | ||
| Test script for the amd-dbgapi unit tests. | ||
|
|
||
| The test binaries are installed under tests/rocdbgapi in the unified ROCm tree. | ||
| They are plain GoogleTest executables, so each one is run directly. | ||
| """ | ||
|
|
||
| import logging | ||
| import os | ||
| import platform | ||
| import shlex | ||
| import subprocess | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
| THEROCK_BIN_DIR = os.getenv("THEROCK_BIN_DIR") | ||
| if not THEROCK_BIN_DIR: | ||
| raise RuntimeError("THEROCK_BIN_DIR environment variable is not set") | ||
|
|
||
| logging.basicConfig(level=logging.INFO, format="%(message)s") | ||
|
|
||
| test_dir = Path(THEROCK_BIN_DIR).resolve().parent / "tests" / "rocdbgapi" | ||
|
|
||
| is_windows = platform.system() == "Windows" | ||
| tests = sorted( | ||
| p | ||
| for p in test_dir.iterdir() | ||
| if p.is_file() and (p.suffix == ".exe" if is_windows else os.access(p, os.X_OK)) | ||
| ) | ||
| if not tests: | ||
| logging.error(f"No test executables found in {test_dir}") | ||
| sys.exit(1) | ||
|
|
||
| failed = [] | ||
| for test in tests: | ||
| cmd = [str(test)] | ||
| logging.info(f"++ Exec [{test_dir}]$ {shlex.join(cmd)}") | ||
| if subprocess.run(cmd, cwd=test_dir).returncode != 0: | ||
| failed.append(test.name) | ||
|
|
||
| if failed: | ||
| logging.error(f"Failed tests: {', '.join(failed)}") | ||
| sys.exit(1) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be great, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a temporary commit now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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