Skip to content

remove test warnings #592

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

Merged
merged 1 commit into from
Apr 21, 2017
Merged

remove test warnings #592

merged 1 commit into from
Apr 21, 2017

Conversation

king6cong
Copy link
Contributor

No description provided.

@Susurrus
Copy link
Contributor

These changes all look good to me, but I don't get warnings on any of these either locally or see any in Travis CI. What warnings are you seeing and with what toolchain?

@king6cong
Copy link
Contributor Author

yes, cargo test or cross test will not show the warnings
this will:
cargo test -- --nocapture

@Susurrus
Copy link
Contributor

Interesting, I would have thought compilation warnings would have been exposed by default running cargo test.

Two things:

  1. Can you also fix the extraneous debug output on line 37 of test/test_fcntl.rs as part of this.
  2. I think we should change from running cargo test to running cargo test -- --nocapture to try to prevent this recurring in the future. It will still return a 0 status code even with warnings. I really think this is something that should be changed in Rust proper. @king6cong would you have any interest in filing a ticket in rust-lang/cargo about this?

@kamalmarhubi
Copy link
Member

Oh that's interesting. Thanks for this!

@Susurrus
Copy link
Contributor

@kamalmarhubi Any opinion on changing our cargo test runs to doing cargo test -- --nocapture assuming cross will allow it?

@@ -35,7 +35,7 @@
//! from [rust-spidev](https://github.com/posborne/rust-spidev).
//!
//! ```
//! #[macro_use] extern crate nix;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be actually able to completely remove this line. This is automatically inserted by the doctest script.

@Susurrus
Copy link
Contributor

cargo test -- --nocapture reveals a bunch of extraneous output however, so I suggest we don't change that.

@king6cong Can you address my comment? I think after that we're good to go.

@king6cong
Copy link
Contributor Author

@Susurrus updated 😃

@Susurrus
Copy link
Contributor

bors r+

bors bot added a commit that referenced this pull request Apr 21, 2017
592: remove test warnings r=Susurrus
@bors
Copy link
Contributor

bors bot commented Apr 21, 2017

Build succeeded

@bors bors bot merged commit 31abe00 into nix-rust:master Apr 21, 2017
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