Refactor example CI setup.#437
Conversation
This change: - Adds many more `build_test` targets to ensure non-tests will still be built and checked. - Some of these are not working and are currently disabled. - Splits each top-level package into a separate CI action so that it's easier to spot failures and ensure other tests continue - Adds a new tag-based way to disable tests: - `no-ci` will skip the test in CI entirely - `no-remote-ci` and `no-local-ci` will skip based on remote execution - `no-linux-ci` and `no-macos-ci` will skip based on the OS
mbland
left a comment
mbland
left a comment
There was a problem hiding this comment.
LGTM, though I have a few questions and suggestions to consider before merging. Nothing ultimately blocking.
| srcs = ["DockerNetworkTest.java"], | ||
| tags = ["manual"], | ||
| tags = [ | ||
| "no-ci", # Consistent NPE |
There was a problem hiding this comment.
What's the benefit of no-ci vs. the 'manual' tag here? Does CI not skip it already? Would it hurt to keep both tags for the sake of local runs?
There was a problem hiding this comment.
As you note, the manual tag stops target pattern expansion. This is being used in the README file for the examples to make them more useful, and is also being used to keep targets from being tested in CI. I wanted to add a new tag to split these two meanings out (which works because the tests query filter does not filter out manual targets).
I've re-added the manual tags I removed: we can later go back and decide which were "don't expand as part of running the examples" and which were "don't expand in tests".
| }, | ||
| tags = ["manual"], | ||
| tags = [ | ||
| "no-local-ci", |
There was a problem hiding this comment.
no-local-ci is just slightly confusing since I normally don't think of CI jobs running locally on my workstation. Would it be better to toggle using no-remote-ci and remote-ci or some similar nomenclature?
There was a problem hiding this comment.
I wanted to make all of the tags negative (it's easier to express the logic that way in the filtering step). Using positive tags would mean that every test needs a linux-ci, macos-ci, remote-ci and local-ci tag (so the default is that the test appears nowhere), whereas using negative tags means the default is to run the test and the tag explains where not to.
If you have a suggestion for replacing no-local-ci I am very willing to entertain it: I was hoping that the -ci part was clear enough that this is "local as respects whereever CI runs".
| - scala | ||
| - swift | ||
| - typescript | ||
| remote_execution: [true, false] |
There was a problem hiding this comment.
Suggestion, not a blocker: It would be nice to use more descriptive strings for values instead of true and false, so that the GitHub Checks job titles look more descriptive than:
CI / linux-test-matrix (cpp, false, false)
CI / linux-test-matrix (cpp, true, false)
CI / linux-test-matrix (cpp, true, true)It would make boolean expressions a little more verbose, but would make the UI a lot easier to follow.
| os_version: "14" | ||
| revision: 4edc10aa8f8ca239699aaf1078832208fb3b7efe | ||
| remote_execution: 'true' | ||
| test_package: |
There was a problem hiding this comment.
I'm not loving the duplication of this list. Isn't there a way to define a YAML list once and reuse it?
Following the examples from https://stackoverflow.com/questions/9254178/is-there-yaml-syntax-for-sharing-part-of-a-list-or-map, something like:
test_packages: &test_packages
- cpp
- csharp
# And so on...
matrix:
test_package: *test_packagesThere was a problem hiding this comment.
I hate that syntax. My original plan was to allow different lists per platform, they just ended up being the same.
I'm going to leave as-is and then we can revisit later if it's a problem.
2 participants
This change:
build_testtargets to ensure non-tests will still be built and checked.no-ciwill skip the test in CI entirelyno-remote-ciandno-local-ciwill skip based on remote executionno-linux-ciandno-macos-ciwill skip based on the OS