Replace native-suite-count file with dynamic test discovery - #11413
Merged
Conversation
… a file test/native-suite-count was a manually-maintained register of the test_* directory count, reconciled against the actual directories by bin/run-tests.sh (as an AMBER verdict) and by a dedicated suite-count-check CI job. The reconciliation only ever guarded the file itself: the check that matters - suites that actually ran vs. the test_* directories on disk - already derives its expected count from a directory walk, so the file added a bookkeeping step to every suite addition/removal without adding signal. Remove the file and everything that existed to keep it honest: - bin/run-tests.sh: drop the canonical-count file read, the count-mismatch AMBER verdict, and the [canonical: x/y] suffix; the verdict lines already carry ran/expected from the directory walk. The shuffle seed suffix stays. - test_native.yml: delete the suite-count-check job and its needs: edges. - Docs (copilot-instructions.md, AGENTS.md, test/README.md) and the test-script comments now describe the count as derived from test/test_* at run time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APCEfNjd1X7ErDHEzT6Dqd
With test/native-suite-count gone, nothing in CI noticed the suite set shrinking: platformio test discovers and runs whatever test_* directories exist, and bin/run-tests.sh derives its expected count from the same walk, so a suite directory lost in a bad rebase or an overzealous cleanup just means fewer suites run - every remaining check stays green. Restore that tripwire git-aware instead of file-based: on pull_request runs, compare the test_* directory list at the PR's merge base against the PR result. A vanished suite fails the job unless its name appears in the PR title, PR body, or a commit message in the PR's range - a deliberate removal satisfies that by stating what it removes; an accidental loss cannot. Other events skip: they have no natural base, and PRs are where accidents arrive. No job depends on this one (a skipped job would skip its dependents). Incidentally: test/ currently holds 47 test_* directories while the deleted count file said 46 - the manual register had already drifted, which is exactly the bookkeeping failure mode this replaces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APCEfNjd1X7ErDHEzT6Dqd
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughNative test suite counts now derive from ChangesNative suite discovery and enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/copilot-instructions.md:
- Line 735: Replace the literal suite counts in the documented RESULT examples
on lines 735 and 738 with the symbolic N/M placeholder used on line 726, while
preserving the existing result wording and statuses.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f8215b1-58d9-43f3-b6d5-aeaae9649ba2
📒 Files selected for processing (8)
.github/copilot-instructions.md.github/workflows/test_native.ymlAGENTS.mdbin/run-tests.shbin/test-lint-unity-exit.shbin/test-state-check.shtest/README.mdtest/native-suite-count
💤 Files with no reviewable changes (1)
- test/native-suite-count
| # AMBER: native-suite-count disagrees with test/ directory count (too high) | ||
| RESULT: AMBER test/ has 24 suite directories but native-suite-count says 99 - update test/native-suite-count after removing suites | ||
| # AMBER: a suite silently went missing on a full run | ||
| RESULT: AMBER 23/24 suites ran (missing: test_radio) - all that ran passed |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove literal suite counts from the examples.
Line 735 contains 23/24, and Line 738 contains 1/24. Replace both values with symbolic placeholders such as N/M, as already used in Line 726.
As per coding guidelines, never state the number of test suites as a literal; discover it from the current test/test_* directories.
Suggested documentation fix
-RESULT: AMBER 23/24 suites ran (missing: test_radio) - all that ran passed
+RESULT: AMBER N/M suites ran (missing: test_radio) - all that ran passed
-RESULT: FILTERED 1/24 suites ran (not run: test_admin_radio test_atak …) - filtered: test_serial
+RESULT: FILTERED N/M suites ran (not run: test_admin_radio test_atak …) - filtered: test_serialAlso applies to: 738-738
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/copilot-instructions.md at line 735, Replace the literal suite
counts in the documented RESULT examples on lines 735 and 738 with the symbolic
N/M placeholder used on line 726, while preserving the existing result wording
and statuses.
Source: Coding guidelines
Shrinking the AMBER cell left the table's column padding inconsistent, which trunk (prettier + markdownlint MD060) rejects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APCEfNjd1X7ErDHEzT6Dqd
develop bumped the count file to 47 (the drift this branch's first commit noted); this branch removes the file entirely in favor of deriving the count from the test_* directories, so the deletion wins. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APCEfNjd1X7ErDHEzT6Dqd
jp-bennett
pushed a commit
that referenced
this pull request
Aug 11, 2026
Resolves test/native-suite-count modify/delete: develop replaced the count file with dynamic test discovery (#11413), so the deletion wins and our 46->47 bump is obsolete. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1
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
3 participants
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.
Summary
Removes the manual
test/native-suite-countfile and replaces it with dynamic detection of test suites. The canonical suite count is now derived on-the-fly fromtest_*directories undertest/, eliminating the need to manually update a static file when adding or removing test suites.Changes
CI/CD (
test_native.yml)suite-count-checkjob tosuite-shrinkage-checkwith new logic:test_*directories between merge base and PR headneeds: suite-count-checkdependency from test jobs (no job depends on this check)Test Runner (
bin/run-tests.sh)test/native-suite-countfileEXPECTED_COUNT=${#ALL_SUITES[@]}canonical_rating()toverdict_suffix()(only includes shuffle seed if applicable)Documentation
.github/copilot-instructions.mdto explain the new schemetest/README.mdto clarify that suite count is derived fromtest_*directoriesAGENTS.mdto reference dynamic detectionbin/test-state-check.shandbin/test-lint-unity-exit.shto clarify they are not counted in the suite totalRemoved
test/native-suite-countfile (no longer needed)Rationale
The previous approach required manually maintaining a count file that could drift from reality. This created two failure modes:
The new approach:
Testing
suite-shrinkage-checkjob🤝 Attestations
https://claude.ai/code/session_01APCEfNjd1X7ErDHEzT6Dqd
Summary by CodeRabbit
Improvements
Documentation