ci: make sure tests fail in CI if gpg or taplo binaries are not found
#5854
Conversation
| @@ -142,6 +142,12 @@ impl TestEnvironment { | |||
| } | |||
| } | |||
|
|
|||
| /// Panic if `CI` environment variable is set to a non-empty value | |||
| pub fn ensure_running_outside_ci(reason: &str) { | |||
There was a problem hiding this comment.
nit: move to module-level function because this does nothing related to TestEnvironment.
| /// Panic if `CI` environment variable is set to a non-empty value | ||
| pub fn ensure_running_outside_ci(reason: &str) { | ||
| let running_in_ci = std::env::var("CI").is_ok_and(|value| !value.is_empty()); | ||
| assert!(!running_in_ci, "Running in CI, {reason}."); |
There was a problem hiding this comment.
nit: I think it's better to add JJ_TEST_FORCE_GPG or something. It might make sense for down-stream packagers to enable this flag.
There was a problem hiding this comment.
I didn't follow the situation and the behavior that you have in mind. My first thought is that if we create such a variable, this could be changed to fail if either CI or JJ_GPG_... is defined.
Or do you want us to use some variable we manually set in our CI workflows?
There was a problem hiding this comment.
It's just about naming. I think explicitly-named variable is better than CI.
There was a problem hiding this comment.
CI is kind of a standard. I think most CI runners set it by default, I've seen it in GitHub and Sourcehut, I think.
Also, do we have a need of separate variables for taplo and gpg?
There was a problem hiding this comment.
CI is kind of a standard. I think most CI runners set it by default
Oh, I didn't know that and assumed we would set CI=1 somewhere.
do we have a need of separate variables for taplo and gpg?
Downstream packagers might want to forcibly enable gpg tests, but not taplo? That wouldn't be a big deal if CI is the standard way to handle this kind of things, though.
There was a problem hiding this comment.
It's less well-documented than I hoped. The best I found so far is https://github.com/actions/runner/pull/374/files.
There are also crates like https://github.com/zkat/is_ci, https://crates.io/crates/ci_info, https://docs.rs/built/0.7.7/built/util/fn.detect_ci.html. Might be a bit of an overkill.
Fixes #5696
Example failure: https://github.com/jj-vcs/jj/actions/runs/13611152082/job/38048476201?pr=5854#step:7:1543
(This can wait; since the CI passes on the PR, we know the problem this PR aims to detect is not occurring, at least at the commit the PR is synced to).