Skip to content

Fix testing #689

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 2 commits into from
Jul 19, 2017
Merged

Fix testing #689

merged 2 commits into from
Jul 19, 2017

Conversation

Susurrus
Copy link
Contributor

Move powerpc-unknown-linux-gnu to Tier 2. And attempt a fix for the epoll tests that seem to fail reliably now.

cc @asomers

@Susurrus
Copy link
Contributor Author

Note that this looks like a big patch series because it's based on #681 because it needs the signalfd fixes there, but only the newest two commits are part of this series.

@Susurrus Susurrus mentioned this pull request Jul 19, 2017
8 tasks

#[test]
pub fn test_epoll_errno() {
// Create a new signalfd to use for testing. This is better than using stdin or other such fd.
let sfd = SignalFd::new(&SigSet::empty()).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

is a SignalFd really the simplest kind of file descriptor that works with epoll? What about plain old files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it matter? Is signalfd overkill or would you just rather reduce cross-dependencies between tests? This seems to suggest you can use it on regular files, so let me give that a shot.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, exactly. I would rather use a plainer file descriptor type, so that this test doesn't depend on signalfd. Sockets would work too.

.travis.yml Outdated
@@ -97,6 +97,9 @@ matrix:
rust: beta

allow_failures:
# There are too many inconsistent failures on this arch to make it Tier 1.
Copy link
Member

Choose a reason for hiding this comment

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

Frankly, I would rather just add DISABLE_TESTS=1 to powerpc. I don't think we're likely to learn much by running them, especially since we can't trust any of their failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, allowing failures would also make this Tier 2 not Tier 3, so good catch!

@Susurrus
Copy link
Contributor Author

Well this failed with signalfd anyways...I'm wondering if this has anything to do with us removing sudo from our CI builds...

@asomers
Copy link
Member

asomers commented Jul 19, 2017

I can reproduce the epoll test failures, but only when using the nightly compiler and only in release mode. The failure is due to unwrapping an Sys(UnknownErrno). However, it happens after libc::epoll_ctl returns 0. So epoll_ctl ought to be returning Ok(). This looks like a compiler bug to me.

@Susurrus
Copy link
Contributor Author

Man, didn't even realize it was the beta that was failing and stable was passing. This passes on nightly (7-16 and 7-18) for me on x86_64-linux-gnu however. I also installed the same beta Rust version on the machine that failed and it succeeded for me. Hmmm...

I'm thinking with should put the beta under allow_failures.

@Susurrus
Copy link
Contributor Author

To elaborate on that, we have builds from yesterday that were still using rustc 1.19.0-beta.4 (deb2ab60b 2017-07-14), but the latest ones use rustc 1.20.0-beta.1 (e93aa3aa8 2017-07-18) and that fails. Can you look into this a bit since you can reproduce it and file a bug against rust? For now I'm going to change beta to be allowed to fail.

@Susurrus
Copy link
Contributor Author

@asomers What do you think? Should we merge it (BTW we can just merge this without bors I think).

@asomers
Copy link
Member

asomers commented Jul 19, 2017

Yes, we should. I also think that you should hop in Gitter so we can talk about this Rust bug.

bors r+

bors bot added a commit that referenced this pull request Jul 19, 2017
689: Fix testing r=asomers

Move `powerpc-unknown-linux-gnu` to Tier 2. And attempt a fix for the epoll tests that seem to fail reliably now.

cc @asomers
@bors
Copy link
Contributor

bors bot commented Jul 19, 2017

@bors bors bot merged commit 9cf6a55 into nix-rust:master Jul 19, 2017
@Susurrus Susurrus deleted the fix_testing branch July 19, 2017 05:57
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.

2 participants