c2rust icon indicating copy to clipboard operation
c2rust copied to clipboard

Catch more compile-time errors in CI

Open kkysen opened this issue 3 years ago • 3 comments

Right now in CI, we just test

cargo build
./scripts/test_translator.py tests/

We should test more:

cargo fmt --check
RUSTFLAGS='-D warnings' cargo clippy --tests --all --all-features # includes cargo build
RUSTDOCFLAGS='-D warnings' cargo doc --all-features --document-private-items
cargo test --all-features
./scripts/test_translator.py tests/
# immunant/c2rust-testsuite CI

Note that we currently don't build with -all-features so, for example, the dynamic-instrumentation feature and that whole crate are not built in CI at all.

Furthermore, we should run the external tests, ./scripts/test_translator.py tests/ and immunant/c2rust-testsuite CI inside of cargo test. This not only simplifies testing workflows, but also hooks into external testing, such as crater runs.

I think we should definitely add these at least by the time we move CI to GitHub Actions instead of Azure Pipelines, but preferably earlier, as cargo clippy is almost passing (https://github.com/immunant/c2rust/issues/474), and once it is, I'd like to keep it that way.

  • [x] cargo fmt --check
  • [x] cargo check --all-features
  • [x] cargo build
  • [x] RUSTFLAGS='-D warnings' cargo check --all-features
  • [x] RUSTFLAGS='-D warnings' cargo build
  • [ ] RUSTFLAGS='-D warnings' cargo clippy --tests --all-features (includes cargo check)
  • [x] RUSTDOCFLAGS='-D warnings' cargo doc --all-features --document-private-items --no-deps
  • [x] cargo test
  • [x] ./scripts/test_translator.py tests/
  • [ ] https://github.com/immunant/c2rust/issues/593

PRs and Tracking Issues:

  • [x] #483
  • [ ] #594
  • [ ] #608
  • [ ] #429

kkysen avatar Jun 30 '22 07:06 kkysen

Furthermore, we should run the external tests, ./scripts/test_translator.py tests/ and immunant/c2rust-testsuite CI inside of cargo test.

I think cargo test should not run immunant/c2rust-testsuite since the latter has complex dependencies. Let's keep it separate.

thedataking avatar Jul 01 '22 09:07 thedataking

Okay, I haven't really looked into the details of what immunant/c2rust-testsuite does, so that might make sense. I do think running the in-repo ./scripts/test_translator.py tests/ as part of cargo test makes sense though, as that doesn't have many dependencies, and for ones there are, like the cross tests, we can just skip testing them if they're not available.

kkysen avatar Jul 01 '22 11:07 kkysen

That sounds reasonable. ./scripts/test_translator.py tests/ tests/ has far fewer dependencies, so that should be okay.

thedataking avatar Jul 01 '22 17:07 thedataking