Skip to content
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

update for new TestDesc::allow_fail field #68

Merged
merged 1 commit into from
Jun 30, 2017

Conversation

dwrensha
Copy link
Contributor

Fixes breakage introduced by rust-lang/rust#42219.

@@ -274,6 +274,7 @@ pub fn make_test(config: &Config, testpaths: &TestPaths) -> test::TestDescAndFn
name: make_test_name(config, testpaths),
ignore: early_props.ignore,
should_panic: should_panic,
allow_fail: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should likely be a field of the Config structure so that it's configurable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is copied from the upstream make_test() function: https://github.com/rust-lang/rust/blob/919c4a6707da3aa2cc9ff63e33057e1e2a90720b/src/tools/compiletest/src/main.rs#L479.

I think what we really want to do is to parse the attributes of the test to determine the intended value of allow_fail, but that would be less trivial of a change. Right now I only care about getting compiletest to run again.

I don't see how it would make sense to use a field of Config, as that struct is shared among all tests.

Choose a reason for hiding this comment

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

I tend to agree that fixing this should take priority over implementing new features, because without this fix this crate is not usable right now.

Maybe a separate ticket can be opened to make this configurable?

@laumann
Copy link
Collaborator

laumann commented Jun 30, 2017

Hi guys, sorry for the delay - I'll merge it now

@laumann laumann merged commit 2740780 into Manishearth:master Jun 30, 2017
@laumann
Copy link
Collaborator

laumann commented Jun 30, 2017

@dwrensha @SergioBenitez @colin-kiegel Do any of you want to be added as maintainers on this repo, in case @Manishearth and I are not responding quick enough?

@dwrensha dwrensha deleted the allow-fail branch June 30, 2017 16:02
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.

4 participants