docs(hipblaslt): add TESTING.md testing strategy document - #10266
Draft
tony-davis wants to merge 7 commits into
Draft
docs(hipblaslt): add TESTING.md testing strategy document#10266tony-davis wants to merge 7 commits into
tony-davis wants to merge 7 commits into
Conversation
hipBLASLt has substantial test machinery spread across a GTest client suite, two TensileLite Python suites, a per-PR sanitizer lane, and two separate CI systems, but nothing that says what any of it is for, which signals actually block a merge, or where the holes are. New contributors cannot tell which test to write, and reviewers cannot tell which check to trust. This adds the component testing strategy document following the ROCm-wide TESTING.md template. hipBLASLt is the pilot component for that template. The document describes current state rather than intended state, so it records several gaps plainly: characterization goldens are enforced in Math CI but silently skipped in the GitHub Actions lane, test tiers above quick apply no filter in the TheRock lane, there is no automated performance regression detection at any cadence, gfx950 never runs the client suite on a PR, and the C++ library is largely unreachable from unit tests for structural reasons that the roadmap section names. Complements the test mechanics documentation in #10205 rather than duplicating it; that PR covers how the client test flow works, this one covers strategy. Co-authored-by: Cursor <cursoragent@cursor.com>
The first draft said the TensileLite characterization goldens are enforced only in Math CI and silently skipped in GitHub Actions. That came from an internal wiki page that predates the Component CI TensileLite coverage workflow, and it is wrong. tox.ini puts syrupy in the base testenv deps, coverage-unit inherits them and runs pytest over the characterization tree, and the GitHub Actions coverage lane runs coverage-unit on any PR touching projects/hipblaslt/tensilelite. So a stale golden fails a GitHub Actions check, and Math CI runs the same tox environment, giving two independent lanes rather than one. The only lane that skips the snapshots is the installed-artifact re-run over share/hipblaslt/tensilelite, whose requirements do not include syrupy. The suite's conftest detects the missing plugin and skips those tests cleanly instead of erroring, so the skip is deliberate and the goldens were already asserted in the source lanes before the artifact was built. Downgrades the corresponding entry in the gap table from medium/high to low/low, restates the roadmap item as making that skip explicit rather than as adding missing enforcement, and adds a paragraph stating plainly where enforcement happens. Co-authored-by: Cursor <cursoragent@cursor.com>
…onfig The first draft took several claims from an internal wiki page without verifying them. Checking each one against the repository and against the Jenkins job definitions showed the wiki was stale in ways that changed the conclusions, not just the details. Performance testing. The draft said there was no automated benchmarking at any cadence and that the ci_perf_job suite had no callers. Both wrong. Math CI runs a perfci job on pull requests that builds the change and a develop reference, benchmarks both with that suite on gfx950, uploads both datasets to rocPTS, and comments on the PR; a performance job does the same daily on gfx942 and gfx950 with the full suite. What is actually missing is a threshold and an alert, since the ingest step only fails on upload errors and neither job gates. Rewrote the section around that distinction. Gating jobs. hipBLASLt's gating set is precheckin, static-analysis and preliminary. The draft said static analysis was not run at all, and said no pull request had ever run the client suite on gfx950. precheckin runs the client suite on gfx90a, gfx942, gfx950 and gfx12 and it gates, so both claims were wrong and the gfx950 risk row came out. Added a description of the three gating jobs, including that preliminary skips itself when nothing under tensilelite touched. Coverage. The enforced floor is fail_under = 75 in pyproject.toml, with the comment there recording that the run measures around 79% and 80% is the target. The draft's "far below that, starting from the low thirties" was wrong. The named-exception files were wrong too: KernelWriter.py, KernelWriterAssembly.py and SolutionStructs/Solution.py all carry per-file floors in the seventies. Also separated the enforced floor from the codecov target, which is project coverage per flag, not patch coverage. Co-authored-by: Cursor <cursoragent@cursor.com>
…rage The document quoted union coverage throughout and let a characterization golden count the same as a unit test. That overstates how well tested TensileLite Python actually is, and it hides the debt the consolidation refactor is supposed to be paying down. A golden proves behavior did not change. It does not prove the behavior is right, and some goldens deliberately pin bugs. So a line reached only by characterization is protected, not verified, and it still owes a real test. The enforced floors cannot see any of this: they measure the union, so converting a line from a golden to a unit test moves no number, and a file can sit at 80% while almost all of it is scaffolding. Threads that distinction through the sections where a reader would otherwise draw the wrong conclusion: the unit testing strategy now has an explicit block on it, the coverage section separates union coverage from the unit-only share, the coverage table lists the split and mutation score as unenforced rows, and the change-type guidance says a golden is not a substitute for a unit test on new code. Also reframes mutation testing as the check on whether the scaffolding would catch anything, rather than as an unrelated pilot, and adds the migration itself to the roadmap: the split summary card already computes the characterization-only statement count, so it can carry a target instead of being a dashboard nobody owns. Two risk rows added for the same reason. The concepts and the terminology come from the repository, in characterization/README.md and tools/coverage_split_summary.py, which already frame the suite as scaffolding with a migration debt. The document was simply not carrying it. Co-authored-by: Cursor <cursoragent@cursor.com>
…ic YAML gate TESTING.md described the client quarantine list but missed six other places that suppress or record known-bad behavior, and omitted the TensileLogic --check-all gate entirely even though it runs on every kernel-generating build. Inventory all seven in one place, sort them by whether they can detect their own fix, and give the logic validation its own section, gate row, and roadmap entry. The four governance rules are proposed as a roadmap item rather than stated as policy, since two mechanisms cannot satisfy them today and adopting them needs team agreement. Related: AIHPBLAS-4196, AIHPBLAS-4049, AIHPBLAS-3575 Co-authored-by: Cursor <cursoragent@cursor.com>
The gating Math CI job named "static-analysis" scans the tree and git log for sensitive words; it is a disclosure gate, not a code analyzer. The document presented it as static analysis coverage, which is misleading in the direction that matters. Replace that claim with what actually runs: no clang-tidy or cppcheck configuration exists for hipBLASLt, CodeQL is weekly and covers only Python and Actions, and the Python linter is narrowed to pyflakes and invoked by no CI job. Add a Static Analysis section, split the gate row into three honest rows, and record the gaps as risks and roadmap items. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
documentation
ci:docs-only
✅ 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. |
The Known Risks and Gaps table carried an Owner column, as the ROCm template prescribes. Per-gap ownership churns faster than this document does, and a stale name in a repository file is worse than no name because it reads as authoritative. Replace it with a Tracking column holding an issue or ticket reference, so ownership and status live where they can be kept current. An empty cell now means the gap is real, acknowledged, and untracked, which is a more useful thing to see than a row of TBD. Three cells are filled today. Apply the same reasoning to the proposed known-bugs governance rule, which had asked each quarantine entry to carry an owner, and note in Owners and Review Cadence that area ownership belongs in the document while per-item ownership belongs on a ticket. Also merge two duplicate mutation-testing rows that the new column made obvious. Co-authored-by: Cursor <cursoragent@cursor.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
JIRA ID : N/A (tracked in #10265)
Motivation
hipBLASLt has a lot of test machinery and no map of it. A GTest client suite with four tiers, two TensileLite Python suites including 98 characterization goldens, coverage floors with a per-file ratchet, a per-PR HOST_ASAN gate, a build-time validation pass over the library logic YAML, and two independent CI systems posting checks onto the same pull request. Nothing in the repository says what any of that is for, which checks actually block a merge, or where the holes are.
That has practical costs today. New contributors cannot tell which kind of test to write, so they default to adding another integration case. Reviewers cannot tell which red check is a real gate and which is informational. And we keep having the "I thought CI was covering that" conversation after the fact, which is the conversation this document exists to prevent.
This is also the pilot for the ROCm-wide TESTING.md template. The ask was for one component to walk the template end to end before every team fills one in, so that the template gets validated against a real component rather than in the abstract. hipBLASLt volunteered.
Technical Details
Adds
projects/hipblaslt/TESTING.md, following the ROCm template section for section. P1: component overview, development workflow, unit and integration strategy, build-time validation of library logic, performance and benchmarking, pre-submit CI gates with the prescribed sub-tables, sanitizer coverage, static analysis. P2: why we test this way, key quality concerns, release validation, dependency handoffs, coverage expectations by change type, improvement roadmap, known risks and gaps, contributor guidance.Four deliberate choices worth flagging for review.
It describes current state, not intended state. The template asks for honesty over polish, and a testing document that overstates coverage is worse than no document. The corrections in the Test Result section below are the substance of this; several of them contradict things that are widely believed.
It separates characterization coverage from unit coverage throughout. Characterization tests are temporary scaffolding that pin current behavior, bugs included, while unit tests assert correct behavior. The enforced floors measure the union of the two and cannot tell them apart, so a module can sit at 80% while almost none of that is verification. The document treats the characterization-only share as debt to be repaid rather than testing that is finished, and positions mutation testing as the only current evidence that a golden would actually catch a change.
It consolidates known-bug handling. Seven mechanisms across the component suppress or record known-bad behavior, and they were never governed as one thing. The document inventories all seven and sorts them by whether the mechanism can detect that the bug has been fixed, which turns out to be the property that separates a healthy suppression from one that rots. It proposes four governance rules as a roadmap item rather than stating them as policy, since two of the mechanisms cannot satisfy them today and adopting them needs team agreement.
The gap table tracks work items rather than owners. The template prescribes an Owner column here, and this deliberately deviates from it. Per-gap ownership churns much faster than a document does, and a stale name in a repository file is worse than no name because it reads as authoritative. The column holds an issue or ticket reference instead, so ownership and status live where they can actually be kept current, and an empty cell honestly means the gap is acknowledged but tracked nowhere. Three of twenty-one rows are filled today. Area-level ownership, which changes slowly, is still named in Owners and Review Cadence. Worth a look in review on both counts: it departs from the template, and filling in more of those cells is the most useful thing anyone could do to this document.
Test Plan
Docs only, no product code or test changes. Validation was accuracy checking rather than execution:
pre-commit run --files projects/hipblaslt/TESTING.md(no markdown hooks are configured in this repo, so all hooks report no files to check).Test Result
Link audit clean on 1020 lines: 54 links, 0 broken anchors, 0 missing relative paths, 0 internal URLs, and all 8 external URLs returning 200. Pre-commit passes. No CI behavior changes, so nothing to benchmark.
Several widely-held claims turned out to be wrong when checked against the tree and against the CI job definitions. They are listed here because they are exactly what review should be double-checking, and because a few of them are load-bearing.
Static analysis is the big one. The Math CI job named
static-analysisis a gating job, but it is not a code analyzer. It scans the working tree and the git log for a list of sensitive words maintained in the CI system, and fails on any match. It is a disclosure gate protecting a public repository, which is a worthwhile thing, but it would not notice a null dereference. Meanwhile there is no.clang-tidyanywhere in hipBLASLt, the repository'sClangTidy.cmakeis never referenced by this component, and CodeQL runs on a weekly schedule againstpythonandactionsonly, so it never sees C++ and never runs on a pull request. The net is that the C++ library gets no static analysis on a PR from any source. On the Python side,tox -e lintis configured but invoked by no CI job, and its[flake8]section setsignore = E, W, which disables all of pycodestyle and leaves the pyflakes checks (themax-line-length = 132on the line above it is therefore inert). If any single correction in this PR deserves scrutiny, it is this one.Golden enforcement. It is widely believed that the characterization snapshots are enforced only in Math CI and skipped in GitHub Actions. Not so.
tox -e coverage-unitinherits syrupy from the base tox environment and runs the characterization tree, and theComponent CI: TensileLite coverageworkflow runs that env on any PR touchingtensilelite/. Two independent lanes assert the goldens. The only lane that skips them is the installed-artifact re-run, whereconftest.pyskips deliberately because the packaged tree does not ship syrupy.Performance automation exists. An earlier draft said there was no automated benchmarking at any cadence and that the
ci_perf_jobsuite had no callers. Both wrong. Theperfcijob builds the change and adevelopreference, benchmarks both on gfx950, uploads both to rocPTS and comments on the PR; aperformancejob does the same daily on gfx942 and gfx950 with the full suite. The real gap is narrower and more fixable than "no automation": there is no threshold and no alert, so a regression only gets caught if someone reads the dashboard.gfx950 does run the client suite on every PR, via the gating
precheckinjob, which covers gfx90a, gfx942, gfx950 and gfx12. The gap is specific to the TheRock GitHub Actions lane. An earlier draft stated the gap as absolute and listed it as a high risk; that row is gone.The coverage numbers. The enforced floor is
fail_under = 75intensilelite/pyproject.toml, and the comment there records that the run measures around 79%. The often-quoted "roughly 28%, lowest of the math libraries" is a different measurement scope. Separately,codecov.ymlconfigures 80% project coverage per flag; there is no patch-coverage target configured anywhere.The named coverage exceptions are no longer uncovered.
KernelWriter.py(78.05%),KernelWriterAssembly.py(73.57%) andSolutionStructs/Solution.py(70.55%) all carry active per-file floors. The genuinely uncovered modules areExperimentalLibrary.pyand much ofTensile/Components/.The library logic validation gate was missing from the document entirely.
TensileLogic --check-allruns as a CMake custom command ahead of codegen, so it executes on every kernel-generating build including local ones, and it is the only thing in the component that validates tuning data rather than code. It now has its own section. Its known-bugs list is the best-structured quarantine we have, and--strict-known-bugs(added in #9355) can detect a stale entry, but the flag defaults off and nothing passes it yet.Suite size. The characterization suite is bigger than the numbers in circulation: 60+ modules and roughly 250 test files behind 98 goldens, not the ~29 modules from when it first landed.
The
--cpu-onlyseam is present ondevelopeven though the PR usually cited for it is still open, so the document points at the test file rather than the PR.Submission Checklist
Risk level
None. Documentation only: no product code, no test code, no CI configuration, no build files. The only risk is that the document is inaccurate, which is what review is for. Please push back hard on anything that does not match your understanding of how a lane behaves; several of these claims are about systems I have been reading rather than running.