Group the tests by what they need, and give the static checks their own directory - #884
Merged
Merged
Conversation
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
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.
Tests were spread across three workflows, the style workflow was running pytest, and two of the
three unittest jobs installed the whole project, torch included, to run checks that only read
files. This sorts them by what they actually need.
Measured
The image job is the only one that installs the project, and it is the only one above a minute.
What each directory needs
test/linttest/shelltest/imagetest/quicktestOnly
test/imageneeds the project installed. Everything else was paying a fullpip install .[quicktest]for nothing, and the install is the entire cost of those jobs.test/lint
New directory for the checks that only read files and never execute anything:
test_bash4_lint.py, moved fromtest/shell, scans the shipped scripts for bash-4-onlyconstructs. Its own docstring already said it "runs in the style job".
test_python_version.py, moved fromtest/config, which dissolves. It asserts that thedeclared python version agrees across pyproject.toml, the Dockerfile and the workflows.
test_every_test_dir_runs.py, new, see below.These are style checks rather than tests of behaviour, so they run in
code-style.yml, whichnow runs the directory instead of naming files. A new static check lands there automatically.
The static and functional halves of the bash 3.2 coverage are now in different directories, so
the two docstrings that cross-reference each other name the full paths.
unittest.yaml
The matrix splits by what the tests need rather than by directory. The install step branches on
needs-project, so one job installs the project instead of two, and the macOS job runstest/shellas a directory rather than naming two files.PYTHON_PATHbecomesPYTHONPATH, exported only where the project was not installed: itprecedes site-packages, so setting it unconditionally would make the image job import the working
tree and leave the installed package untested.
timeout-minutesdrops from 180 to 20 on both jobs, now that a run takes about a minute. Theheadroom is for a cold pip cache, which is what a pull request sees until a run on dev has
populated one.
Two bugs caught in review, one of them by the first CI run
The matrix collapsed to a single job and
test/imagenever ran. GitHub merges anincludeentry into every existing combination when none of its keys is a matrix dimension, so two such
entries overwrite each other rather than creating two jobs. The first CI run on this branch
showed it: a 24 second "unit tests" job, which cannot be the image tests, and no second ubuntu
job at all.
testsis a real dimension again, which makes eachincludea filter.test/lint/test_every_test_dir_runs.pyguards both halves: everytest/<name>holding tests mustbe named by some workflow, expanding
test/${{ matrix.tests }}through the matrix, and at leastone
includekey must be a matrix dimension. Verified against the broken matrix twice: exactlythose two tests fail and nothing else.
PYTHONPATHwas shadowing the installed package, as above.Unrelated CI speedup, found while looking at the timings
code-style.ymlranuv run --project src --extra style ruff check src, which resolved andinstalled all 28 project dependencies, torch included, to run a linter that needs none of them.
That was the four minutes. It now uses
--no-project --with ruff, the same formCONTRIBUTING.md already documents. Ruff still reads
[tool.ruff]fromsrc/pyproject.toml:--show-settingsreportslinter.line_length = 120, which exists nowhere else.The tests now also run on pushes to dev
Not for coverage, since everything lands through a pull request and that event tests the merge
ref. For the pip cache: a run restores caches from its own branch, the default branch, or a pull
request's base branch. Your upstream default branch is
stableand pull requests targetdev,so with no run on
devthis workflow's cache only ever existed at pull-request scope, which noother pull request can read, and every one of them re-downloaded torch.
cancel-in-progressisnow limited to pull requests, because a cancelled run saves no cache.
Note for reviewers
The visible check names change, because the matrix values did:
test/configis gone andtest/shellis new. Any branch protection rule naming the old checks needs updating.