-
Notifications
You must be signed in to change notification settings - Fork 654
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
Skip failing tests for Linux/MIPS/PowerPC #590
Conversation
If CI is skipping some tests on these architectures, I think that makes them Tier 2, not Tier 1. |
@asomers The README has details on what the tiers mean. For Tier 1 it means the platforms are built and tested in CI and failures should never happen. Tier 2 means there is no automated testing, but the code should at least compile. For Tier 3 it means that testing is done, but we don't guarantee that they pass (admittedly Tier 2 and 3 should be swapped). |
Ideally we don't have any platforms under Tier 3 and hopefully I can remove that distinction soon. |
@Susurrus for the three platforms in question, you're proposing that only some of the tests will be run in CI. If they tests aren't all run, then I don't think we can call it Tier 1. Tier 1.1 perhaps, if we want to use floats, but not Tier 1. And currently the Tier 3 definition says that we don't even guarantee that the build will pass. Until we change that guarantee, we shouldn't swap Tier 2 and Tier 3. |
It won't always be possible to run all tests in CI due to various issues with Travis or Qemu or other possible issues. I don't think a distinction between "all" and "almost all" really makes much difference. The point of stating tiers is so that users (and I guess developers as well) have expectations of the state of the code and its rough quality. Platforms in Tier 1 are well supported, Tier 2 less so, and Tier 3 the least. Platforms not supported are at your own risk so to speak. And you bring up a good point about Tier 3, we should separate tests and builds because Tier 3 should be guaranteed to build but don't necessary have their tests run during CI. |
@kamalmarhubi It doesn't look like my |
weird, I tried it out locally. |
3a77c3b
to
ff4676f
Compare
@kamalmarhubi Yeah, it worked correctly when I tested it locally using |
I think the problem is that cross is testing things and since it's in qemu it might not be receiving the |
Oh good point. |
This is relevant: cross-rs/cross#77 |
@kamalmarhubi Good find. That PR looks pretty close to finished so hopefully we can push it through to the next |
Sent cross-rs/cross#91 so hopefully we'll get this functionality sooner than later. |
We should actually also try to upstream our |
Looks like cross is a little hard to test here because of it's need for a release .zip uploaded to GitHub, which is why the most recent commit broke everything. It'll be easiest to block on this until cross-rs/cross#91 gets released. |
03e983a
to
5f7a01e
Compare
8b847f6
to
9d8f735
Compare
Seems that pretty much all aio tests fail on x64 musl builds.
@asomers @kamalmarhubi Could you guys look over this? Seems good to me and I'd like to merge. |
@@ -6,52 +6,57 @@ language: rust | |||
services: docker | |||
sudo: required | |||
|
|||
# This is the Rust channel that build jobs will use by default but can be | |||
# overridden on a case by case basis down below | |||
rust: 1.13.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to use a default rust value? It makes the matrix section more duplicative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use the default, you end up with an "extra" test run. You can see how there's two x86-64-unknown-linux-gnu
runs that execute right now. This fixes that problem. Looks like I didn't specify that in the commit message, which I can do.
@@ -26,6 +26,7 @@ fn poll_aio(mut aiocb: &mut AioCb) -> Result<()> { | |||
// bindings. So it's sufficient to check that AioCb.cancel returned any | |||
// AioCancelStat value. | |||
#[test] | |||
#[cfg_attr(all(target_env = "musl", target_arch = "x86_64"), ignore)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So all of these tests fail in x86_64 musl but pass in i686 musl? Weird. Does cross use qemu to run i686 tests? I would think that it wouldn't need to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You bring up a good point. i don't think cross
runs things in QEMU based on this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I've filed an issue (cross-rs/cross#107) in order for this to be made more clear.
bors r+ |
590: Skip failing tests for Linux/MIPS/PowerPC r=asomers These should be issues with QEMU itself and not problems on the actual hardware, but I want to get some upstream tickets filed before merging this. I'm also not sure if this syntax is correct on the `#cfg`, so this is testing that as well.
Build failed |
This issue is due to a bug in |
cross is fixed. bors r+ asomers |
590: Skip failing tests for Linux/MIPS/PowerPC r=Susurrus These should be issues with QEMU itself and not problems on the actual hardware, but I want to get some upstream tickets filed before merging this. I'm also not sure if this syntax is correct on the `#cfg`, so this is testing that as well.
Build succeeded |
These should be issues with QEMU itself and not problems on the actual hardware, but I want to get some upstream tickets filed before merging this. I'm also not sure if this syntax is correct on the
#cfg
, so this is testing that as well.