Improve the test script and CI pipeline #405
Conversation
Use the Bash `set -e` option to make the test script fail if a command fails. While we are at it add sanity check commands that call the compiler and `cargo`.
There is no obvious reason to run the fuzzer in a subprocess. Remove the subprocess from the fuzzing code. While we are at it change the code comment to state what it does instead of mentioning the CI vms.
There is no obvious reason to run the fuzzer in a subprocess. Remove the subprocess from the fuzzing code.
tcharding
force-pushed
the
05-11-test-script
branch
from
560ded4 to
06fba01
Compare
|
Removed last patch, I guess that is why we are using 1.58 for fuzzing and not stable? |
tcharding
changed the title
In an effort to improve test coverage without being inefficient add a new env variable `DO_FEATURE_MATRIX` to the test script. This is how we do it over in `rust-secp256k1`, it enables good test coverage while not having to run the whole matrix in every CI job. Add cargo calls for each example because the current way we run the examples does not work for those who configure the build output directory to something other than the default.
Be a good UNIX hacker and exit the test script explicitly with a success value.
As we do in `rust-secp256k1` use a string that includes 'Nightly' in name of the nightly test.
In the `Tests` CI job run the feature matrix tests. Also run these tests for 4 toolchains, stable, beta, nightly, and 1.41.1
Rename the integration test job to `IntTests` now that we do not run the full matrix of unit tests in this job. Simplify the yaml by removing the strategy section since we only use a single toolchain in this job.
tcharding
force-pushed
the
05-11-test-script
branch
from
06fba01 to
ed51323
Compare
tcharding
changed the title
| @@ -1,5 +1,7 @@ | |||
| #!/bin/sh -ex | |||
|
|
|||
| set -e | |||
There was a problem hiding this comment.
This is a big bug! I wonder how it ever worked till now
There was a problem hiding this comment.
I guess we were getting some false positives in the CI runs. rust-bitcoin/contrib/test.sh doesn't have this either :)
| cargo run --example sign_multisig | ||
| cargo run --example verify_tx > /dev/null | ||
| cargo run --example psbt | ||
| cargo run --example xpub_descriptors |
There was a problem hiding this comment.
Why manually list examples instead of run-parts? Is it because of the extra args to each example? We need to remember to add examples here every time.
There was a problem hiding this comment.
Its so the script can be run locally, but yes as you say there is a maintenance cost. Is it worth it, I don't know? I configure target-dir to be a global build cache so there is no target directory when I build so the examples don't run.
| run-parts ./target/debug/examples | ||
| # Run all the examples | ||
| cargo build --examples | ||
| cargo run --example htlc --features=compiler |
There was a problem hiding this comment.
nit: There is a white space at the end of this line.
There was a problem hiding this comment.
ooo, means I did not read the diff before pushing, bad Tobin. Thanks man, I'll fix it.
There was a problem hiding this comment.
I read the diff and I missed this too, oops! Thanks Sanket.
Make an effort to improve the test script, taking inspiration for the test script in
rust-secp256k1. Since Bash can, at times, be a bit obscure, I made extra effort to do very simple discreet patches.First part of the PR is fixes to the test script, second part is improvements to the CI pipeline.