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

feat: basic support for tests #94

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gswirski
Copy link

This implements #93

Tests are essentially a binary file in target/debug/deps/library_name folder. That binary accepts test names as arguments. This PR uses --harness option as the library_name (required), and an optional --test option to pass arguments to the test harness binary.

@iamrecursion
Copy link

I can confirm that this works perfectly for my use-case, and makes my life significantly easier.

Copy link
Owner

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Cool, thanks! I definitely think this is worth having, although I have one request for the impl: basically I don't think we need separate --harness and --test arguments; if you want to run a specific test case in an integration test file, you can just use a trailing -- and pass a filter string that way, e.g:

$ cargo instruments -t time --test my_integration_test -- test_case1

(I'm imagining that we get rid of the --harness and use --test to do what --harness does currently.)

Doing it this way makes the code simpler and more consistent, and removes the need to have two different command line arguments, which will hopefully make it clearer how things work.

If you're interested in making these tweaks let me know, otherwise I'm happy to do it.

thanks again!

src/opt.rs Outdated
@@ -56,6 +56,14 @@ pub(crate) struct AppConfig {
#[structopt(long, group = "target", value_name = "NAME")]
bench: Option<String>,

/// Test harness target to run
Copy link
Owner

Choose a reason for hiding this comment

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

Per my top-level comment I would call this argument --test, and then I would want to add some more documentation, explicitly pointing out that this only runs integration tests, and that if you want to filter a specific test than you should pass the test name to the target after --.

Copy link
Author

Choose a reason for hiding this comment

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

Updated the code, will work on docs next. Just a note that technically it doesn't have to be an integration test. All tests generate their binaries. When you run cargo test, you'll see something like

[...]
Running unittests src/main.rs (target/debug/deps/cargo_instruments-3cabce0a041cfeed)

which means that you can do:

cargo instruments -t alloc --test cargo-instruments -- opt::tests::features

Copy link
Owner

Choose a reason for hiding this comment

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

okay that's great. In an ideal world then maybe you should be able to omit the target after --test, and we would run the "default test target", if such a thing exists, and if we can figure out how to get it from cargo? This might involve some splunking in the cargo crate internals to figure out what they do when you run cargo test.

@gswirski
Copy link
Author

gswirski commented Oct 1, 2023

I've added some docs to the README.

Skipping the test harness name is as easy as checking if this array has exactly one element: https://github.com/cmyr/cargo-instruments/pull/94/files#diff-0b979b6de560b29c1d97336878db56179224aa21d96fe099630c6628479ed020R153

Unfortunately, I couldn't figure out how to make structopts detect --test without a value. I won't have time to work on this pull request any more. Please feel free to take over this as you see fit.

@gswirski gswirski requested a review from cmyr October 5, 2023 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants