Skip to content

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
#[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