Skip to content
This repository was archived by the owner on Dec 29, 2021. It is now read-only.

cargo test --release should test the release binary not the debug one #86

Closed
ghost opened this issue Jan 30, 2018 · 6 comments
Closed

Comments

@ghost
Copy link

ghost commented Jan 30, 2018

Here is my motivation or use case:
I'm busy writing a solver for a game and I'd like to test it using assert_cli. In release mode the solver takes around 2 minutes to run. In debug it takes more than 20 minutes. (I've never waited longer.) For this reason, I only want to test the binary built in release mode. At the moment, this is not possible as assert_cli doesn't respect the --release flag.

@epage
Copy link
Collaborator

epage commented Feb 2, 2018

Options for handling this

Anyone have any better ideas of how to expose this to users?

@mikerite which works better for your use case?

@azriel91
Copy link
Contributor

The options that make sense to me would be:

  • Default to running the command with the active compilation profile.
  • (Maybe) Allow users to override the profile, if they really want to.

That way, for this same line of code: assert_cli::Assert::main_binary().unwrap()

  • cargo test would run cargo run --quiet --
  • cargo test --release would run cargo run --release --quiet --

But if the user does something like:

assert_cli::Assert::main_binary()
    .with_profile(Profile::Debug)
    .unwrap()

then it would always omit the "--release" flag; if they used .with_profile(Profile::Release) then it would always include it.

I'm not sure that the explicit user override is necessary — not sure how often the use case would come up. I find that if I want to test a separate compilation profile, then I'd run compile everything with that profile, instead of a mix and match, but then again I'm not the world 😅.

I'm keen to have a go at this.

@epage
Copy link
Collaborator

epage commented Mar 26, 2018

Thanks for the feedback!

Do we have a feel for how important customizing the profile is or other parts of running cargo? If we can avoid writing code that is never run, the better.

If we do need to support it, an interesting angle on the design is should the API allow with_profile to be called with `command? If so, how would they interact? If not, how do we prevent it?

Another fun trade off. We are working on a revamp of assert_cli's API, taking in a lot of lessons we've learned.

  • How much do we work on fixing problems in assert_cli before the rewrite?
    • Obviously, if its directly causing people problems and there isn't a reasonable workaround, we should address it
    • Anything past that is more grey
  • How much design work do we do before the re-write?
    • I feel like we should minimize the redesign work but we should work out the requirements so we don't back ourselves into another corner with the revamp I'm working on.

(you can preview the ideas I'm experimenting with in my branch. Note that I will do force pushes to that branch)

@azriel91
Copy link
Contributor

azriel91 commented Mar 26, 2018

Hm, based on:

I think defaulting to the current compilation profile would produce code that is used by people. Not implementing the "user may choose the profile" will make it much easier (don't have to store the potential command + args, then rebuild later).

Looking at the rewrite, if we just do the first part — default to using the current profile — it should be pretty straightforward to implement whether before or after the rewrite. The second part, I think it's not too difficult to do before the rewrite, but it would be quite difficult to do after — cannot insert arguments in between other arguments.

If there isn't a compelling reason to allow the user to change the compilation profile for the tested binary, then the current state of the rewrite is good, otherwise we'd have to have a builder or something to store the profile and construct the Command later, which is quite different to what it looks like now.

@azriel91
Copy link
Contributor

#97 implements the first part, and I think it's easy to see that it would easily carry over to the current rewrite. I won't attempt to do the second part just yet, seeing that we're not sure we need it.

azriel91 added a commit to azriel91/assert_cli that referenced this issue Mar 27, 2018
@epage
Copy link
Collaborator

epage commented Mar 27, 2018

Closed by #97. If someone has the use case for customized profile (or other customization), then please go ahead and open distinct issues for us to discuss.

@epage epage closed this as completed Mar 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants