artichoke / strftime-ruby

⏳ Ruby `Time#strftime` parser and formatter
https://crates.io/crates/strftime-ruby
MIT License
12 stars 0 forks source link

Add code coverage tests #21

Closed lopopolo closed 2 years ago

lopopolo commented 2 years ago

I think we might be able to reuse some of the same harness from rust-asn1: https://github.com/alex/rust-asn1/blob/afe141f6df60dee51fd5d78884361beaa46a8122/.github/workflows/ci.yml#L101-L170

Having 100% coverage seems like a good goal to strive for given the complexity of the parsing.

x-hgg-x commented 2 years ago

Here what I was using for locally testing coverage:

RUSTFLAGS="-C instrument-coverage" cargo test --lib
grcov **/*.profraw --source-dir . --binary-path target/debug -t html --branch --ignore-not-existing -o target/coverage
lopopolo commented 2 years ago

@x-hgg-x do you want to take a stab at wiring this up in CI? We can chat about whether we would want such a check to be blocking or not and what threshold we're looking to maintain.

x-hgg-x commented 2 years ago

I think we should at least check if the number of missed lines doesn't increase, but with the possibility of bypassing the check when we add a new derive for example.

Here is a working script:

export LLVM_PROFILE_FILE="strftime-%m.profraw"
export RUSTFLAGS="-C instrument-coverage"

# Unstable feature: https://github.com/rust-lang/rust/issues/56925
export RUSTDOCFLAGS="-C instrument-coverage -Z unstable-options --persist-doctests target/debug/doctests"

cargo +nightly test --lib
cargo +nightly test --doc

# Produces a folder with a nice html view for manual inspection
grcov strftime*.profraw --source-dir . --binary-path target/debug -t html --filter covered -o target/coverage

# Produces a machine-readable json
# { "children": {...}, "coveragePercent": 98.48, "linesCovered": 1559, "linesMissed": 24, "linesTotal": 1583, "name": "" }
grcov strftime*.profraw --source-dir . --binary-path target/debug -t covdir --filter covered -o target/coverage.json

However I don't know how to check it in CI, since we need to keep the previous coverage results.

x-hgg-x commented 2 years ago

Maybe we can do something similary to the gh-pages branch, where a bot will push the last coverage result file to a separate branch after a pull request is merged.

lopopolo commented 2 years ago

oh that sounds cool, but I think it would require some significant rework. rustdoc.yaml is centrally managed across all of Artichoke's Rust repositories and is currently set to squash and overwrite.

If you'd like, maybe I can set up an S3 bucket and we can publish there?

lopopolo commented 2 years ago

However I don't know how to check it in CI, since we need to keep the previous coverage results.

Another option is to run the code coverage job against both the PR branch and against trunk in the same workflow and diff those results.

x-hgg-x commented 2 years ago

Another option is to run the code coverage job against both the PR branch and against trunk in the same workflow and diff those results.

This solution should work and is easier to implement.

If you'd like, maybe I can set up an S3 bucket and we can publish there?

It would be nice if we could upload the static html somewhere for manual inspection, and have a badge in the readme which has a link for it (like the codecov badge for https://github.com/time-rs/time).

lopopolo commented 2 years ago

Here is a working script:

This is cool! I played around with it and some bits that were missing:

cargo install grcov
rustup install nightly
rustup +nightly component add llvm-tools-preview

This is the HTML as of the latest trunk: coverage.zip

I'll set up an S3 bucket. Tracking here:

lopopolo commented 2 years ago

One thing I forgot to mention, the machine readable coverage.json from main can be fetched at https://codecov.artichokeruby.org/strftime-ruby/coverage.json.

x-hgg-x commented 2 years ago

This is the json from the html version, which does not contain detailed information with lines.

Fixed in #40.

lopopolo commented 2 years ago

It looks like cloudfront isn't setting a TTL on the badges which means GitHub's proxy is caching them forever.

I'll take a look at the cloudfront terraform, purge the cloudfront cache, and purge the GitHub cache. I found https://github.com/sbts/github-badge-cache-buster or I can add a query parameter to the image link in the README to bust the cache.

lopopolo commented 2 years ago

41 is merged and infra changes are all applied and up at https://github.com/artichoke/project-infrastructure/pull/336.

Also new is the landing page at https://codecov.artichokeruby.org/.

x-hgg-x commented 2 years ago

Do we also need to add a link on the home page for the coverage.json file for discoverability ?

lopopolo commented 2 years ago

If you want to PR that at project infra I can apply and merge in the morning. You mean adding a link on the index page of the codecov website right?

x-hgg-x commented 2 years ago

Yes, another link next to the Coverage Report link.

Associated PR: https://github.com/artichoke/project-infrastructure/pull/357.