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

Make test file pattern configurable and not force test_* naming convention. #50

Closed
sagiegurari opened this issue Dec 26, 2017 · 9 comments
Assignees
Milestone

Comments

@sagiegurari
Copy link
Owner

Based on @azriel91 feedback in #44

@sagiegurari sagiegurari self-assigned this Dec 26, 2017
@sagiegurari sagiegurari added this to the 0.9.1 milestone Dec 26, 2017
@sagiegurari
Copy link
Owner Author

Users can now override the test_* pattern for integration tests using a new env var.

CARGO_MAKE_KCOV_INTEGRATION_TEST_FILE_PATTERN = "my_tests_*"

@azriel91
Copy link
Contributor

hmm, I was thinking of a more "it works out of the box magically", as in given a crate with some integration tests, cargo make would be able to discover them without needing them to be changed. The part that I was looking at in #44 was coverage which was looking for integration test executables to feed to kcov, which might be solved for free if we solve the "find test executables, but not the application executable" problem (#51).

@sagiegurari
Copy link
Owner Author

will try, but rustc doesn't make life easier with all those hashs and stuff.
won't be closing this issue for now, and will try to see.
I do prefer a scripting solution over rust, as kcov is invoked via script so have everything in one place, and I prefer that the rust code you wrote is not inside cargo-make itself as it is not task specific aware.
bottom line, i'll think about this one more.

@sagiegurari sagiegurari modified the milestones: 0.9.1, 0.9.2 Dec 28, 2017
sagiegurari added a commit that referenced this issue Dec 29, 2017
@sagiegurari
Copy link
Owner Author

Fixed in 0.9.2, all tests will now run and no configuration is needed or special naming convesions.

@azriel91
Copy link
Contributor

azriel91 commented Dec 30, 2017

I tried out 0.9.2, and the cargo make --no-workspace workspace-coverage flow runs every file under target/debug/deps, which includes the interactive apps as well as the <dependency>.d and <dependency>.rlib files. I think we still need the CARGO_MAKE_CRATE_FS_NAME check for the virtual workspace.

A quick scripting solution to reduce the noise could be to run things that match:

find "${BINARY_DIR}" -type f -executable -maxdepth 1 | grep -e "-[0-9a-f]\{16\}$"

Then we still have to solve the "cargo make --no-workspace workspace-coverage runs interactive applications" problem, but at least the "run all tests" problem goes away.

Random thought, does BINARY_DIR need to be target/debug/deps, or can it just be target/debug? since all the test binaries appear to be there anyway, and the interactive app doesn't have the -hashcode suffix, it could solve that problem too.

Gotta test on Mac OS X as well (there may be other extensions missing), but I don't have one right now Updated grep expression to match binaries with 16 character hash at the end. I know that that could be overly strict, depending on whether the binary names will change

azriel91 added a commit to azriel91/cargo-make that referenced this issue Dec 30, 2017
This should restrict the files run during coverage to the test
executables relevant to the workspace.

Issue sagiegurari#50
azriel91 added a commit to azriel91/cargo-make that referenced this issue Dec 30, 2017
This prevents the need to discover binaries relevant to the current
coverage collection operation, which has proven to be difficult / error
prone.

Issue sagiegurari#50
azriel91 added a commit to azriel91/cargo-make that referenced this issue Dec 30, 2017
This prevents the need to discover binaries relevant to the current
coverage collection operation, which has proven to be difficult / error
prone.

Issue sagiegurari#50
@sagiegurari
Copy link
Owner Author

Its actually being executed in target/debug and not target/debug/dep or am I mistaken?
I do add some hash to the end so current solution is ok for you as well. no?

@azriel91
Copy link
Contributor

azriel91 commented Jan 1, 2018

Its actually being executed in target/debug and not target/debug/dep or am I mistaken?

Yeap that's right, I think I was based off an older master when typing that

I do add some hash to the end so current solution is ok for you as well. no?

It doesn't work for this set up:

virtual_workspace
  |- app/app1
  |   |- src/main.rs
  |   |- tests/start_and_exit.rs  # contains tests
  |
  |- crate/lib1
      |- src/lib.rs               # contains tests

The start_and_exit test runs cargo run for the app1 binary, which works if the working directory is app1. However the coverage-kcov task runs all of the binaries from every crate directory, and cargo run (which is run by the start_and_exit-123456 test) won't work for the lib1 directory since it doesn't have a binary.

Restricting the match pattern to be the crate name wouldn't work for the app1 crate, since the binary name is start_and_exit-123456 instead of app1-start_and_exit-123456 so we can't really discover the integration test binary names (sad face)

@sagiegurari
Copy link
Owner Author

when developing libraries, I have both unit tests (test files in the src folder) and integration tests (in the tests folder).
when developing an app, I only have unit tests because of this reason. Not because of kcov, but because it is not something I can test easily using code outside the app.

@sagiegurari
Copy link
Owner Author

Closing this issue. If you think we can do something reopen it and we will see what more can be done.
Like I said, for cli apps, integration tests are tricky in general and not just for coverage.

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

No branches or pull requests

2 participants