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

Modified binary directory and test executable discovery for coverage #55

Closed
wants to merge 1 commit into
base: master
from

Conversation

3 participants
@azriel91
Contributor

azriel91 commented Dec 30, 2017

This should restrict the files run during coverage to the test executables relevant to the workspace.

Issue #50

@codecov-io

This comment has been minimized.

codecov-io commented Dec 30, 2017

Codecov Report

Merging #55 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #55   +/-   ##
=======================================
  Coverage   94.46%   94.46%           
=======================================
  Files          33       33           
  Lines        5200     5200           
=======================================
  Hits         4912     4912           
  Misses        288      288

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17bb1b8...2a922c2. Read the comment docs.

@sagiegurari

This comment has been minimized.

Owner

sagiegurari commented Dec 30, 2017

Thanks, I'll test it as well locally.

@sagiegurari sagiegurari self-assigned this Dec 30, 2017

sagiegurari added a commit that referenced this pull request Dec 30, 2017

@sagiegurari

This comment has been minimized.

Owner

sagiegurari commented Dec 30, 2017

There are multiple issues with your changes, for example BINARY_DIR isn't defined and the patterns don't work for me that well.
I just pushed a new commit which takes ideas from your changes but also does few more stuff.
I'm not publishing a new version yet, so if you want to test it as well (please do), take the coverage-kcov task definition from the internal toml and put in some external Makefile.toml and run it and tell me if it works good for you.

@azriel91

This comment has been minimized.

Contributor

azriel91 commented Dec 30, 2017

BINARY_DIR isn't defined
ah rookie move 🤦‍♂️

take the coverage-kcov task definition from the internal toml and put in some external Makefile.toml and run it and tell me if it works good for you

not sure if I'm just doing something wrongTM but for some reason copy pasting the whole [tasks.coverage-kcov] and editing the script in my project's Makefile.toml doesn't seem to update it, though I know it worked when I overrode [tasks.workspace-members-coverage.env]. I do see [cargo-make] INFO - Using Build File: Makefile.toml in the output at the beginning of the run. Anyway I just used cargo install --force --git ... / cargo install --force for local.


I just tried out the current master (17bb1b8), and it doesn't work with my integration test, but it made me realize a few things:

  • find wants -maxdepth 1 to come before -type f

  • In cargo make --no-workspace coverage, the coverage-kcov task is run per workspace member, which means using the find to choose what executables to run doesn't make sense — tests from every other crate are run under kcov, whereas we just need to run the executables for the current crate.

    Side note, this causes test binaries that expect the working directory to be the crate directory to fail

Thinking a bit more, instead of using the find to find binaries, we run cargo test under kcov, which gives these benefits:

  • We don't have to figure out what binaries belong to the current crate we are collecting coverage for
  • If the binary names change, it doesn't matter to us because cargo handles what binaries to run

That makes the coverage flow work a lot better for my repository, with a caveat:

coverage_unmerged_vs_merged

The last binary that is executed will have the overarching stats on the kcov index page, but I think that problem may already have existed anyway. The [merged] stats are still correct, and that's the main one I look at anyway so I'm not fussed.

@azriel91 azriel91 force-pushed the azriel91:issue-50/do-not-restrict-test-naming-convention branch from fa42793 to 3b33a2d Dec 30, 2017

Use cargo test to execute binaries during coverage collection
This prevents the need to discover binaries relevant to the current
coverage collection operation, which has proven to be difficult / error
prone.

Issue #50

@azriel91 azriel91 force-pushed the azriel91:issue-50/do-not-restrict-test-naming-convention branch from 3b33a2d to 2a922c2 Dec 30, 2017

@azriel91

This comment has been minimized.

Contributor

azriel91 commented Dec 30, 2017

I pushed a change to this branch, hopefully it works for you 🤞

@sagiegurari

This comment has been minimized.

Owner

sagiegurari commented Dec 30, 2017

That's a really awesome idea. I'll test it tomorrow and if works good, i'll merge it in.
I'll run few tests with standalone, workspace and without dependencies and see if everything checks out.

@sagiegurari

This comment has been minimized.

Owner

sagiegurari commented Jan 1, 2018

Really tried to make this one work, but with no success.
I either get 0 coverage or in other cases get core dumps:

> kcov --include-pattern=/projects/rust/src/ target/coverage cargo test

error: process didn't exit successfully: `rustc -vV` (signal: 11, SIGSEGV: invalid memory reference)

Which is strange since running the rustc -vV works good, so it doesn't play well via kcov

> rustc -vV

rustc 1.24.0-nightly (3bee2b44c 2017-12-16)
binary: rustc
commit-hash: 3bee2b44cfd5610ad0346f0179602d91a8dedfd0
commit-date: 2017-12-16
host: armv7-unknown-linux-gnueabihf
release: 1.24.0-nightly
LLVM version: 4.0
@azriel91

This comment has been minimized.

Contributor

azriel91 commented Jan 1, 2018

Ah sadness, seems to be SimonKagstrom/kcov#153 / SimonKagstrom/kcov#212. Does it work if you use kcov --verify <rest of args>?

@azriel91

This comment has been minimized.

Contributor

azriel91 commented Jan 2, 2018

Just retried this, running cargo test under kcov doesn't actually work, the coverage stats I had were false metrics from a non-clean target/debug directory 😭

@azriel91

This comment has been minimized.

Contributor

azriel91 commented Jan 2, 2018

Linking related issues:

There is cargo metadata which could be used to smarten the fuzzy search based on the package (you can figure out all the binaries from there), but I'm not keen on adding yet another dependency that requires compilation to CI

@sagiegurari

This comment has been minimized.

Owner

sagiegurari commented Jan 2, 2018

I subscribed to both issues in cargo. I think once resolve we should revisit this, but until than I'm not sure there is much we can do.

@azriel91

This comment has been minimized.

Contributor

azriel91 commented Jan 2, 2018

cool I'll close this episode, and do something to make the build happy in the meantime. Thanks a lot for your efforts!

@azriel91 azriel91 closed this Jan 2, 2018

@azriel91 azriel91 referenced this pull request Jan 3, 2018

Closed

workspace support redesign #44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment