Skip to content

fix: CI never ran tests for 3 of 4 packages, or even triggered on them - #33

Merged
Adithya-Thonse merged 3 commits into
TexasInstruments:mainfrom
musicalplatypus:pr/ci-run-all-package-tests
Aug 5, 2026
Merged

fix: CI never ran tests for 3 of 4 packages, or even triggered on them#33
Adithya-Thonse merged 3 commits into
TexasInstruments:mainfrom
musicalplatypus:pr/ci-run-all-package-tests

Conversation

@musicalplatypus

Copy link
Copy Markdown
Contributor

Problem

Multiple independent peer reviews of PRs opened this session flagged the same gap repeatedly: none of the new regression tests added to tinyml-tinyverse, tinyml-modelzoo, or tinyml-modeloptimization/torchmodelopt actually run in CI. Two compounding problems:

  1. .github/workflows/test-modelmaker.yml's push/pull_request path filters only watched tinyml-modelmaker/** — a PR touching only tinyml-tinyverse, tinyml-modelzoo, or tinyml-modeloptimization (which is most of this session's PRs) never triggered the workflow at all.
  2. Even when the workflow does run, it only ever executes tinyml-modelmaker/tests/ (four specific files via named Tier 1/2/3 steps). The other three packages have no test-running step whatsoever.

Fix

  • Restored the tinyml-tinyverse/**, tinyml-modelzoo/**, and tinyml-modeloptimization/** path filters alongside tinyml-modelmaker/**, so the workflow actually triggers for PRs touching those packages.
  • Added a test step for each of the three previously-uncovered packages, guarded by a find-based existence check (not a plain test -d tests) since none of these tests/ directories exist yet on main — an empty/missing directory makes pytest exit non-zero ("no tests collected"), which would otherwise fail CI on this package until the first PR adding tests to it merges. The guard lets this land safely now and starts actually running tests the moment any PR adds them.

Deliberately out of scope

I considered also adding a broader "run everything under tinyml-modelmaker/tests/" sweep, since several existing test files there (test_constants.py, test_dataset_utils.py, test_nas_support.py, etc.) aren't wired into any of the three named Tiers and are therefore also never run. I dropped that: it surfaces 17 pre-existing failures unrelated to anything in this session (confirmed even the current, unmodified Tier 1 step alone already fails 2/447 tests locally), which would immediately turn this workflow red on merge. That's a real, separate gap, but out of scope for a fix that should land safely without breaking CI.

Testing

Verified locally: the find-based guard correctly skips tinyml-tinyverse, tinyml-modelzoo, and torchmodelopt on this branch (no test content exists yet on main) and would correctly run pytest once a PR adds real test files there. YAML syntax validated.

🤖 Generated with Claude Code

t5fkg8d44d-beep and others added 2 commits August 4, 2026 20:13
Multiple independent Opus peer reviews of PRs opened this session flagged
the same underlying gap repeatedly: none of the new regression tests
added to tinyml-tinyverse, tinyml-modelzoo, or
tinyml-modeloptimization/torchmodelopt actually run in CI.

Two separate problems compound this:

1. .github/workflows/test-modelmaker.yml's push/pull_request path filters
   only watched 'tinyml-modelmaker/**' -- a PR touching only
   tinyml-tinyverse, tinyml-modelzoo, or tinyml-modeloptimization (which is
   most of this session's PRs) never triggered the workflow to run at all,
   regardless of what test steps existed.

2. Even when the workflow does run, it only ever executes tests under
   tinyml-modelmaker/tests/ (four specific files via named Tier 1/2/3
   steps). tinyml-tinyverse, tinyml-modelzoo, and torchmodelopt have no
   test-running step whatsoever -- any regression test added there is
   silently never checked on push or PR.

Fixed by:
- Restoring the tinyml-tinyverse/**, tinyml-modelzoo/**, and
  tinyml-modeloptimization/** path filters alongside tinyml-modelmaker/**,
  so the workflow actually triggers for PRs touching those packages.
- Adding a test step for each of the three previously-uncovered packages.
  Each is guarded by a find-based existence check (not a plain `test -d
  tests`) because none of these tests/ directories exist yet on
  upstream/main -- an empty or missing directory makes pytest exit
  non-zero ("no tests collected"), which would otherwise fail CI on this
  package until the first PR adding tests to it merges. The guard lets
  this land safely now and start actually running tests the moment any
  PR (this session's or otherwise) adds them.

Deliberately NOT adding a broader "run everything under
tinyml-modelmaker/tests/" sweep beyond the existing four named Tier
files, despite tinyml-modelmaker itself having several test files (e.g.
test_constants.py, test_dataset_utils.py, test_nas_support.py) not
wired into any Tier and therefore also never run: a full sweep surfaces
17 pre-existing failures unrelated to anything in this session (verified
even the current, unmodified Tier 1 step alone already fails 2/447 tests
locally), which would immediately turn this workflow red on merge. That
gap is real but is its own, separate, pre-existing issue -- out of scope
for a CI-triggering/coverage fix that should land safely.

Verified locally: the find-based guard correctly skips tinyml-tinyverse,
tinyml-modelzoo, and torchmodelopt on this branch (none of their tests/
directories have any content yet on upstream/main) and would correctly
run pytest once a PR adds real test files there.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Independent Fable peer review of this PR caught an in-scope gap: the
pull_request path filters omitted '.github/workflows/test-modelmaker.yml'
itself (the push filters have had it all along -- an inherited asymmetry,
not one this PR introduced). Consequence: a PR that only changes this
workflow -- including this very PR -- never triggers the CI it modifies,
so workflow changes merge unvalidated. Given this PR's whole purpose is
"CI doesn't trigger when it should," that omission belongs in scope.

One added path glob, mirroring the push filter.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@musicalplatypus

Copy link
Copy Markdown
Contributor Author

An independent peer review of this PR caught an in-scope gap. Pushed a follow-up commit (9775450):

The pull_request path filters omitted .github/workflows/test-modelmaker.yml itself (the push filters have always had it -- an inherited asymmetry). Consequence: a PR that only changes this workflow, including this very PR, never triggers the CI it modifies, so workflow changes merge unvalidated. Given this PR's stated purpose is "CI doesn't trigger when it should," that belongs in scope. One added path glob, mirroring the push filter.

The same review also verified the load-bearing claims: for pull_request events GitHub evaluates both path filters and the workflow file from the PR's merge ref (base+head), so once this merges, the other open PRs pick up the new triggers/steps on their next synchronize event with no rebase -- though merging this does not retroactively spawn runs, so each open PR needs one push/synchronize to get its first run under the new workflow. Dependency coverage for the not-yet-existing test suites was also verified (tinyml-tinyverse's pyproject declares onnxruntime/torcheval/etc. and is installed without --no-deps, and every new test file across the open PR branches imports only stdlib + already-installed packages).

Both CodeRabbit and Fable reviews flagged that the find-based guard only
matched test_*.py, so a package whose only tests use pytest's other
default discovery pattern (foo_test.py) would be silently skipped in CI.
Extended all three guards to match both patterns; verified the guard
runs for a lone foo_test.py and still skips an empty tests/ directory.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Adithya-Thonse
Adithya-Thonse merged commit 0ac3d3c into TexasInstruments:main Aug 5, 2026
0 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants