Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

include coverage instrumentation #797

Closed
wants to merge 14 commits into from
22 changes: 20 additions & 2 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ jobs:
- name: install rust
uses: dtolnay/rust-toolchain@master
with:
components: rustfmt, clippy
components: rustfmt, clippy, llvm-tools-preview
toolchain: ${{ matrix.rust }}

- name: caching
Expand All @@ -52,11 +52,29 @@ jobs:
- name: run checks & tests
run: RUST_MATRIX=${{ matrix.rust }} cargo xtask run-checks ${{ matrix.test }}

- name: Install Grcov
run: cargo install grcov
Copy link
Collaborator

@Luni-4 Luni-4 Sep 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To speed up CI and save grcov binary in cache we can use the provided binary

    - name: Install grcov
      env:
        GRCOV_LINK: https://github.com/mozilla/grcov/releases/download
        GRCOV_VERSION: v0.8.18
      run: |
        curl -L "$GRCOV_LINK/$GRCOV_VERSION/grcov-x86_64-unknown-linux-musl.tar.bz2" |
        tar xj -C $HOME/.cargo/bin

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


- name: Run grcov
run: grcov . --binary-path target/debug/deps/ -s . -t cobertura --branch --ignore-not-existing --ignore 'target/**' --ignore '../**' --ignore '/*' -o coverage.xml
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can upload the coverage results on codecov that better shows which lines of code are still not covered by tests, so we can replace coverage.xml with lcov.info and add -t lcov instead of -t cobertura. Perhaps we can also replace target/debug/deps with only target/debug/. We can also decide to exclude some directories from code coverage analysis adding more --ignore path directories. This should also speed CI

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! I was using a vendorless approach, but Codecov is great.

Is the Burn repo already set on Codecov?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not necessary to set up anything, just the action is sufficient


- name: Code Coverage Report
uses: irongut/CodeCoverageSummary@v1.3.0
with:
filename: coverage.xml
badge: true
fail_below_min: false
format: markdown
hide_branch_rate: false
hide_complexity: false
indicators: true
output: both
thresholds: '60 80'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can replace this action with codecov action:

    - name: Codecov upload
      uses: codecov/codecov-action@v3
      with:
        files: lcov.info


check-typos:
runs-on: ubuntu-latest
steps:
- name: checkout
uses: actions/checkout@v4

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can re-insert this empty line, it seems a typo

- name: run spelling checks using typos
run: cargo xtask run-checks typos
35 changes: 27 additions & 8 deletions xtask/src/runchecks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
//! - `typos` to check typos in the source code
//! - `examples` to check the examples compile

use std::collections::HashMap;
use std::env;
use std::process::{Child, Command, Stdio};
use std::str;
Expand Down Expand Up @@ -62,6 +63,16 @@ fn rustup(target: &str) {

// Define and run a cargo command
fn run_cargo(command: &str, first_params: &[&str], second_params: &[&str], error: &str) {
run_cargo_with_envs(command, first_params, second_params, error, HashMap::new());
}

fn run_cargo_with_envs(
command: &str,
first_params: &[&str],
second_params: &[&str],
error: &str,
envs: HashMap<&str, &str>,
) {
// Print cargo command
println!(
"\ncargo {} {} {}\n",
Expand All @@ -75,6 +86,7 @@ fn run_cargo(command: &str, first_params: &[&str], second_params: &[&str], error
.arg(command)
.args(first_params)
.args(second_params)
.envs(envs)
.stdout(Stdio::inherit()) // Send stdout directly to terminal
.stderr(Stdio::inherit()) // Send stderr directly to terminal
.spawn()
Expand Down Expand Up @@ -106,14 +118,21 @@ fn cargo_install(params: &[&str]) {
);
}

// Run cargo test command
fn cargo_test(params: &[&str]) {
// Run cargo test command with coverage instrumentation
fn cargo_test_with_coverage(params: &[&str]) {
let envs = [
("RUSTFLAGS", "-Cinstrument-coverage"),
("CARGO_INCREMENTAL", "0"),
("LLVM_PROFILE_FILE", "coverage-%p-%s.profraw"),
];

// Run cargo test
run_cargo(
run_cargo_with_envs(
"test",
params,
&["--color=always", "--", "--color=always"],
"Failed to run cargo test",
envs.iter().cloned().collect(),
);
}

Expand Down Expand Up @@ -161,7 +180,7 @@ fn build_and_test_no_std(crate_name: &str) {
cargo_build(&["-p", crate_name, "--no-default-features"]);

// Run cargo test --no-default-features
cargo_test(&["-p", crate_name, "--no-default-features"]);
cargo_test_with_coverage(&["-p", crate_name, "--no-default-features"]);

// Run cargo build --no-default-features --target wasm32-unknown-unknowns
cargo_build(&[
Expand Down Expand Up @@ -206,10 +225,10 @@ fn burn_core_std() {
println!("\n\nRun checks for burn-core crate with tch and wgpu backend");

// Run cargo test --features test-tch
cargo_test(&["-p", "burn-core", "--features", "test-tch"]);
cargo_test_with_coverage(&["-p", "burn-core", "--features", "test-tch"]);

// Run cargo test --features test-wgpu
cargo_test(&["-p", "burn-core", "--features", "test-wgpu"]);
cargo_test_with_coverage(&["-p", "burn-core", "--features", "test-wgpu"]);
}

// Test burn-dataset features
Expand All @@ -220,7 +239,7 @@ fn burn_dataset_features_std() {
cargo_build(&["-p", "burn-dataset", "--all-features"]);

// Run cargo test --all-features
cargo_test(&["-p", "burn-dataset", "--all-features"]);
cargo_test_with_coverage(&["-p", "burn-dataset", "--all-features"]);

// Run cargo doc --all-features
cargo_doc(&["-p", "burn-dataset", "--all-features"]);
Expand All @@ -237,7 +256,7 @@ fn std_checks() {
cargo_build(&["--workspace", "--exclude=xtask"]);

// Test each workspace
cargo_test(&["--workspace"]);
cargo_test_with_coverage(&["--workspace"]);

// Check format
cargo_fmt();
Expand Down