[Github Action] cli-test: improve naming summary #1448
Conversation
savil
force-pushed
the
savil/improve-cli-tests
branch
from
21e1006 to
559150c
Compare
savil
force-pushed
the
savil/improve-cli-tests
branch
from
559150c to
8da9320
Compare
.github/workflows/cli-tests.yaml
Outdated
| run-devbox-json-tests: false | ||
| - run-devbox-json-tests: true | ||
| - is-main: "is-main" | ||
| run-project-tests: "project-tests" |
There was a problem hiding this comment.
shouldn't this be project-tests-off?
.github/workflows/cli-tests.yaml
Outdated
| @@ -131,8 +130,8 @@ jobs: | |||
| env: | |||
| # For devbox.json tests, we default to non-debug mode since the debug output is less useful than for unit testscripts. | |||
| # But we allow overriding via inputs.example-debug | |||
| DEVBOX_DEBUG: ${{ (!matrix.run-devbox-json-tests || inputs.example-debug) && '1' || '0' }} | |||
| DEVBOX_RUN_DEVBOX_JSON_TESTS: ${{ matrix.run-devbox-json-tests }} | |||
| DEVBOX_DEBUG: ${{ (matrix.run-project-tests == 'not-project-tests' || inputs.example-debug) && '1' || '0' }} | |||
There was a problem hiding this comment.
shouldn't this be project-tests-off?
There was a problem hiding this comment.
I totally agree the labels suck, but I really have trouble following our rules (Many of which I wrote myself - lol)
Alternative:
- Create two different workflows that both call a job in a shared workflow.
- The shared workflow takes inputs that decide what to run (what OS, and what scope of tests).
So the jobs in non-main can be named:
- Basic tests on mac
- All tests on linux
On main:
- All tests on (mac|linux)
(under the hood, each of these is just calling some shared jobs in another workflow which is never triggered directly).
This should really simplify names and also remove much of the exclude logic which is hard to grok.
| # Run tests on: | ||
| # 1. the oldest supported nix version (which is 2.9.0? But determinate-systems installer has 2.12.0) | ||
| # 2. nix version 2.17.0 which introduces a new code path that minimizes nixpkgs downloads. | ||
| # 3. latest nix version (currently, that is 2.17.0, so omitted) | ||
| nix-version: ["2.12.0", "2.17.0"] | ||
| exclude: | ||
| - is-main: false | ||
| - is-main: "not-main" |
There was a problem hiding this comment.
I agree that improving the labels is good, but lines like this make my head spin :(
There was a problem hiding this comment.
@mikeland73 some refactoring like that seems called for. I'll circle back to doing that in a bit.
Will land this for now, so at least we get rid of boolean labels.
.github/workflows/cli-tests.yaml
Outdated
| @@ -131,8 +130,8 @@ jobs: | |||
| env: | |||
| # For devbox.json tests, we default to non-debug mode since the debug output is less useful than for unit testscripts. | |||
| # But we allow overriding via inputs.example-debug | |||
| DEVBOX_DEBUG: ${{ (!matrix.run-devbox-json-tests || inputs.example-debug) && '1' || '0' }} | |||
| DEVBOX_RUN_DEVBOX_JSON_TESTS: ${{ matrix.run-devbox-json-tests }} | |||
| DEVBOX_DEBUG: ${{ (matrix.run-project-tests == 'not-project-tests' || inputs.example-debug) && '1' || '0' }} | |||
Summary
Motivation
The name summary for
cli-testsin the Github PR page is:This isn't very helpful.
This PR:
booleansand changes their true/false states to explicit namesdevbox-json-teststoproject-testsfor brevity.How was it tested?
will observe the testscripts pass...
Now, it looks like:
which is much more understandable.