Magically improve coverage #6345

ryoqun · 2019-10-14T14:54:45Z

Problem

There is an outdated workaround. And logging macros are red by default, which is a kind of eyesore..

Summary of Changes

Now that the upstream bug (rust-lang#45511) is fixed. Remove this workaround!
Also, enable logging with maximum level and discard 'em all just for more greens with no extra effort! :D

Now that the upstream bug (rust-lang#45511) is fixed. Remove this workaround!

mvines · 2019-10-14T14:59:40Z

cc: rust-lang/rust#45511

mvines · 2019-10-14T15:20:58Z

Weird, cargo +nightly-2019-10-03 test --target-dir target/cov --lib --all --exclude solana-local-cluster didn't like this at all -- it just died. It works for you locally @ryoqun?

ryoqun · 2019-10-16T03:14:32Z

Sorry, it seems that this doesn't work out-of-box locally too.

Heh, it turns out this isn't free lunch for coverage greens.... First, I needed a fix in the upstream `librocksdb-sys' crate.. rust-rocksdb/rust-rocksdb#339, which in turn would be blocked by our #5507 even if the PR is accepted. (This may be my system dependant issue.)

Sure long way...

ryoqun · 2019-10-16T03:16:20Z

Also, I'll split the RUST_LOG thing from this PR.

ryoqun · 2019-10-16T04:57:39Z

Also, I'll split the RUST_LOG thing from this PR.

Sorry, I was completely mistook... After, some degree of investigation, it's found that the workaround is still needed. Even grcov uses this as a black magic with no justification or explanation. So I restored it and refreshed the comment. a82a494

ryoqun · 2019-10-16T05:13:56Z

cc: rust-lang/rust#51705

ryoqun · 2019-10-16T05:15:42Z

scripts/coverage.sh

@@ -14,13 +14,14 @@ reportName="lcov-${CI_COMMIT:0:9}"

 if [[ -n $1 ]]; then
  crate="--package $1"
+  shift


To forward the rest to cargo test at https://github.com/solana-labs/solana/pull/6345/files#diff-c29225768574b8edd4f3608f595c29f7R41, shift $1 here.

ryoqun · 2019-10-16T05:19:04Z

scripts/coverage.sh

-_ cargo +$rust_nightly test --target-dir target/cov --lib $crate
+RUST_LOG=solana=trace _ cargo +$rust_nightly test --target-dir target/cov --lib --no-run $crate "$@"
+# shellcheck disable=SC2086 #
+RUST_LOG=solana=trace _ cargo +$rust_nightly test --target-dir target/cov --lib $crate "$@" 2> /dev/null


Separated compile and test for easier debugging because the messages from cargo build would be also discarded altogether otherwise.

Why hide stderr here? 2> /dev/null?

Otherwise, tons of tons of logs will be printed on the terminal, after all we're running all test with trace level. Maybe, better off to note that as a comment?

Or, should we stash the logs to 2> $(mktemp)? I think this is overdue because we're in the coverage run. It's unlikely we need to debug using this build.

Ah, I see. Yes there are times when the build only fails in a coverage run in CI (we had an occurrence of this today in fact!) so being able to view the log could be helpful.

We don't need anything fancy like mktemp here though. Instead just redirect to a fixed file, coverage-stderr.log, then use https://github.com/solana-labs/solana/blob/master/ci/upload-ci-artifact.sh to include the log file as a buildkite artifact for easy access by anybody who wants it

Thanks for explanation done 98005bf..

By the way, I'm guessing for the log file to be ranges of X00MB.. Let's see if its filesize is too big, or not... If that's concern, I'll just compress it via bz2.

Yay, build passed.

Heh, I was pessimistic a bit, it seems.

@mvines Hi, are ~100MB artifacts a bit concern? Should compress?

artifact: target/cov/coverage-stderr.log -rw-r--r-- 1 buildkite-agent buildkite-agent 94182845 Oct 16 19:03 target/cov/coverage-stderr.log + buildkite-agent artifact upload target/cov/coverage-stderr.log 2019-10-16 19:05:27 INFO Found 1 files that match "target/cov/coverage-stderr.log" 2019-10-16 19:05:27 INFO Creating (0-1)/1 artifacts 2019-10-16 19:05:28 INFO Uploading artifact 8e17d76a-93ea-4883-99d9-3606292791b4 target/cov/coverage-stderr.log (94182845 bytes) 2019-10-16 19:05:31 INFO Successfully annotated build

Oh wow. Sure please gzip it. I don't expect this to be used very often so the extra decompression step for the user should be no big deal

it really shrank!

-rw-r--r-- 1 buildkite-agent buildkite-agent 10056926 Oct 16 23:51 target/cov/coverage-stderr.log.gz + buildkite-agent artifact upload target/cov/coverage-stderr.log.gz 2019-10-16 23:52:52 INFO Found 1 files that match "target/cov/coverage-stderr.log.gz" 2019-10-16 23:52:52 INFO Creating (0-1)/1 artifacts 2019-10-16 23:52:52 INFO Uploading artifact 98e02c81-9b3f-456d-95ee-4826660c7cb3 target/cov/coverage-stderr.log.gz (10056926 bytes) 2019-10-16 23:52:54 INFO Successfully annotated build

ryoqun · 2019-10-16T05:21:32Z

@mvines Hi, I updated this PR; in short, workaround is still needed.. Please have a look if there is some time! I hope CI passes with more greens this time, really...

codecov · 2019-10-16T19:06:08Z

Codecov Report

❗ No coverage uploaded for pull request base (master@79e32c9). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master   #6345   +/-   ##
========================================
  Coverage          ?   69.4%           
========================================
  Files             ?     219           
  Lines             ?   46946           
  Branches          ?       0           
========================================
  Hits              ?   32597           
  Misses            ?   14349           
  Partials          ?       0

mvines · 2019-10-16T23:23:22Z

ci/test-coverage.sh

+stderr_log=target/cov/coverage-stderr.log
+compressed_stderr_log="${stderr_log}.gz"
+gzip < "$stderr_log" > "$compressed_stderr_log"
+upload-ci-artifact "$compressed_stderr_log"


How about this instead:

gzip target/cov/coverage-stderr.log upload-ci-artifact target/cov/coverage-stderr.log.gz

yeah, It looks nicer; ccafb52.

Just that I'm too fond of redirections and pipes.. :p

mvines

looks good to me, thanks!

ryoqun · 2019-10-16T23:56:24Z

Yay!

Delete old workaround and magically improve coverage

cef3e6e

Now that the upstream bug (rust-lang#45511) is fixed. Remove this workaround!

mvines added the CI Pull Request is ready to enter CI label Oct 14, 2019

solana-grimes removed the CI Pull Request is ready to enter CI label Oct 14, 2019

ryoqun added 2 commits October 16, 2019 13:47

Restore workaround with updated comment; sadly still needed.

a82a494

Separate for easier debugging; also forward args

550918c

ryoqun changed the title ~~Delete old workaround and magically improve coverage~~ Magically improve coverage Oct 16, 2019

ryoqun commented Oct 16, 2019

View reviewed changes

mvines added the CI Pull Request is ready to enter CI label Oct 16, 2019

solana-grimes removed the CI Pull Request is ready to enter CI label Oct 16, 2019

Redirect coverage stderr to file

98005bf

Compress right before uploading to save space

dba30e2

mvines reviewed Oct 16, 2019

View reviewed changes

Simplify

ccafb52

mvines added the automerge Merge this Pull Request automatically once CI passes label Oct 16, 2019

mvines approved these changes Oct 16, 2019

View reviewed changes

solana-grimes merged commit b7b71b3 into solana-labs:master Oct 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Magically improve coverage #6345

Magically improve coverage #6345

ryoqun commented Oct 14, 2019

mvines commented Oct 14, 2019

mvines commented Oct 14, 2019

ryoqun commented Oct 16, 2019 •

edited

Loading

ryoqun commented Oct 16, 2019

ryoqun commented Oct 16, 2019 •

edited

Loading

ryoqun commented Oct 16, 2019

ryoqun Oct 16, 2019

ryoqun Oct 16, 2019 •

edited

Loading

mvines Oct 16, 2019

ryoqun Oct 16, 2019

mvines Oct 16, 2019

ryoqun Oct 16, 2019

ryoqun Oct 16, 2019

mvines Oct 16, 2019

ryoqun Oct 16, 2019

ryoqun commented Oct 16, 2019

codecov bot commented Oct 16, 2019 •

edited

Loading

mvines Oct 16, 2019

ryoqun Oct 16, 2019

mvines left a comment

ryoqun commented Oct 16, 2019

Magically improve coverage #6345

Magically improve coverage #6345

Conversation

ryoqun commented Oct 14, 2019

Problem

Summary of Changes

mvines commented Oct 14, 2019

mvines commented Oct 14, 2019

ryoqun commented Oct 16, 2019 • edited Loading

ryoqun commented Oct 16, 2019

ryoqun commented Oct 16, 2019 • edited Loading

ryoqun commented Oct 16, 2019

Choose a reason for hiding this comment

ryoqun Oct 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoqun commented Oct 16, 2019

codecov bot commented Oct 16, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvines left a comment

Choose a reason for hiding this comment

ryoqun commented Oct 16, 2019

ryoqun commented Oct 16, 2019 •

edited

Loading

ryoqun commented Oct 16, 2019 •

edited

Loading

ryoqun Oct 16, 2019 •

edited

Loading

codecov bot commented Oct 16, 2019 •

edited

Loading