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

fix: Remove unrelated output in main_binary #50

Merged
merged 3 commits into from
Oct 17, 2017

Conversation

epage
Copy link
Collaborator

@epage epage commented Oct 13, 2017

To help in validating this, tests have been added for main_binary and cargo_binary.

Fixed #45

@epage epage requested a review from killercup October 13, 2017 00:53
@epage
Copy link
Collaborator Author

epage commented Oct 13, 2017

I'm a bit baffled at the failure. stable and nightly succeeded. beta failed with

---- readme_sect_example_line_41 stdout ----
	thread 'readme_sect_example_line_41' panicked at 'failed to read dependencies: Error(Toml(Error { inner: ErrorInner { kind: Custom, line: None, col: 0, message: "missing field `root`", key: [] } }), State { next_error: None, backtrace: None })', /checkout/src/libcore/result.rs:906:4

Copy link
Collaborator

@killercup killercup left a comment

Choose a reason for hiding this comment

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

Is this a known bug with either rust beta or skeptic?

@@ -12,6 +12,9 @@ categories = ["development-tools::testing"]
keywords = ["cli", "testing", "assert"]
build = "build.rs"

[[bin]]
name = "assert_fixture"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This binary is built only when building assert_cli itself, right? I.e., not when using it as a dependency. Is there any chance people might cargo install this? We could add a internal-tests feature and specify required-features for the bin. Or, can we add bins depending on cfg(test) in some other way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This binary is built only when building assert_cli itself, right? I.e., not when using it as a dependency.

I think I've heard depending on something doesn't pull in the bins.

Is there any chance people might cargo install this?

If they did, it wouldn't do them much good.

We could add a internal-tests feature and specify required-features for the bin. Or, can we add bins depending on cfg(test) in some other way?

The problem with relying on features is main_binary can't enable the feature.

@@ -365,7 +368,7 @@ impl Assert {
/// ```
pub fn unwrap(self) {
if let Err(err) = self.execute() {
panic!("{}", err);
panic!("{}", err.display_chain());
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@epage
Copy link
Collaborator Author

epage commented Oct 13, 2017

I'm a bit baffled at the failure. stable and nightly succeeded. beta failed with

Is this a known bug with either rust beta or skeptic?

I've run into this with liquid also (beta, also depends on skeptic). At least on slack, no one else has run into this. I've not created a skeptic issue yet or checked on IRC.

@epage
Copy link
Collaborator Author

epage commented Oct 17, 2017

Reported this to Skeptic
budziq/rust-skeptic#65

`clap` couldn't be used for command line arguments.  I didn't want to
add it as a dependency for clients, which meant it had to be optional.
The problem is that `main_binary` and `cargo_binary` run with optional
features enabled.

Note: you cannot use `main_binary` or `cargo_binary`
- With `Environment::empty` because the executable can't be found
- In skeptic tests (like the README) because the working dir is changed
We use cargo run behind the scenes to run the main binary but that
should only be an implementation detail.

Fixes assert-rs#45
@epage
Copy link
Collaborator Author

epage commented Oct 17, 2017

Since there is no negative effect (loss of coverage, inability to make binaries, etc), going to go ahead and merge.

@epage epage merged commit f94a996 into assert-rs:master Oct 17, 2017
@epage epage deleted the fix/main_binary branch October 17, 2017 23:02
@budziq
Copy link

budziq commented Oct 19, 2017

Is this a known bug with either rust beta or skeptic?

@killercup - it was a skeptic bug due to change in how cargo beta produces Cargo.lock. It's now fixed in crates.io 0.13.2 (in a sane way not parsing Cargo.lock)

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

Successfully merging this pull request may close these issues.

3 participants