Added ignore-sgx for appropriate tests in src/test #60386
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
rasendubi
reviewed
Apr 30, 2019
src/test/run-pass/cfg/cfg-family.rs
Outdated
| @@ -10,3 +8,7 @@ pub fn main() { | |||
| #[cfg(unix)] | |||
| pub fn main() { | |||
| } | |||
|
|
|||
| #[cfg(not(any(windows, unix)))] | |||
There was a problem hiding this comment.
Doesn't this miss the point of test? (Same for target_family test.)
There was a problem hiding this comment.
What's your suggestion for making the test suite pass on non-windows non-unix platforms? It seems silly to me to add an ignore statement for every single such platform that may be added to Rust.
There was a problem hiding this comment.
what exactly is the point of this test? I guess it's to test that you have either windows or unix (except when you don't, hence the ignores?)
In that case, perhaps we want to add some kind of "meta ignore" statement instead?
There was a problem hiding this comment.
But, honestly, adding some ignore statements doesn't seem terrible for the time being. =) How often do we add such platforms anyway? And it's only two more tests.
There was a problem hiding this comment.
what exactly is the point of this test?
I think it's to test that cfg(unix) works, as in, that the compiler correctly sets the configuration based on the target specification.
nikomatsakis
requested changes
Apr 30, 2019
There was a problem hiding this comment.
This seems good but:
- the git history needs a bit of cleanup before we can merge this. It'd be nice to remove (e.g.) the stray printlns from the history and other such accidents. We also avoid merging PRs with merge commits. Are you familiar with
git rebase? - I agree with @rasendubi that the
#[cfg(not(any(windows, unix))]annotations seem to defeat the point of the test. I'm not sure what's a better more general fix, but I think for now it makes sense to just add a// ignore-fooannotations (after all, we do it on a ton of other tests, so I don't see why it's important not to do it on this particular test, particularly if we don't have a compelling alternative).
src/test/run-pass/cfg/cfg-family.rs
Outdated
| @@ -10,3 +8,7 @@ pub fn main() { | |||
| #[cfg(unix)] | |||
| pub fn main() { | |||
| } | |||
|
|
|||
| #[cfg(not(any(windows, unix)))] | |||
There was a problem hiding this comment.
what exactly is the point of this test? I guess it's to test that you have either windows or unix (except when you don't, hence the ignores?)
In that case, perhaps we want to add some kind of "meta ignore" statement instead?
src/test/run-pass/cfg/cfg-family.rs
Outdated
| @@ -10,3 +8,7 @@ pub fn main() { | |||
| #[cfg(unix)] | |||
| pub fn main() { | |||
| } | |||
|
|
|||
| #[cfg(not(any(windows, unix)))] | |||
There was a problem hiding this comment.
But, honestly, adding some ignore statements doesn't seem terrible for the time being. =) How often do we add such platforms anyway? And it's only two more tests.
1f56d59 to
2b9b826
Compare
nikomatsakis
requested changes
May 7, 2019
There was a problem hiding this comment.
As far as I can tell, those two tests still ought to be changed.
cc @alexcrichton -- I suspect you know something about those tests, do you agree these changes are defeating the point of the tests? I'm not 100% sure.
|
It looks like the original changes aren't here any more (the tests have been updated). FWIW our test suite really wasn't ever written (or will be...) to run on every single platform under the sun. As can be seen from the other annotations in these files getting the test suite running on a particular platform involves just sprinkling a lot of directives throughout the codebase, and naturally this will regress quite often since we're not testing it on CI itself. I wish we had a better system for managing this but we aren't currently employing anything. |
|
@nikomatsakis I have reverted those tests to just adding the ignore directives. |
|
☔ The latest upstream changes (presumably #60775) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@Goirad ok thanks! Sorry for the delay. r=me once rebased. |
|
@nikomatsakis That's alright! And everything is rebased |
|
@bors r+ rollup |
|
📌 Commit 4e7ac47 has been approved by |
bors
added a commit
that referenced
this pull request
May 18, 2019
Added ignore-sgx for appropriate tests in src/test These are all the tests that make sense to ignore when targeting fortanix-unknonw-sgx, at least in test/runpass. Other suites not yet covered.
|
☀️ Test successful - checks-travis, status-appveyor |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These are all the tests that make sense to ignore when targeting fortanix-unknonw-sgx, at least in test/runpass. Other suites not yet covered.