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

Fix error message in test, run tests on AppVeyor #120

Merged
merged 4 commits into from
Jul 27, 2018

Conversation

fpoli
Copy link
Contributor

@fpoli fpoli commented Jul 27, 2018

This fixes tests on Travis and enable tests on AppVeyor.

However, tests on AppVeyor are failing: https://ci.appveyor.com/project/fpoli/compiletest-rs/build/1.0.1
It's probably some Window-related issue (path separators...). I suggest to open a new issue, as I can't debug this easily.

Closes #113

@fpoli fpoli changed the title Fix error message in test, run test on AppVeyor Fix error message in test, run tests on AppVeyor Jul 27, 2018
@laumann
Copy link
Collaborator

laumann commented Jul 27, 2018

This should fix #113, I think.

@fpoli
Copy link
Contributor Author

fpoli commented Jul 27, 2018

Yes, but it doesn't solve the failing tests on AppVeyor

@laumann
Copy link
Collaborator

laumann commented Jul 27, 2018

No, I think I can fix that though. You're right, it's related to spaces in paths.

@laumann
Copy link
Collaborator

laumann commented Jul 27, 2018

Ok, I think if you do:

--- a/test-project/tests/tests.rs
+++ b/test-project/tests/tests.rs
@@ -9,7 +9,7 @@ fn run_mode(mode: &'static str) {
 
     config.mode = cfg_mode;
     config.src_base = PathBuf::from(format!("tests/{}", mode));
-    config.link_deps();
+    config.target_rustcflags = Some("-L target/debug -L target/debug/deps".to_string());
     config.clean_rmeta();
 
     compiletest::run_tests(&config);

It should work. I know it's a hack, but I'd rather have these tests running at the moment, than trying to solve the bigger underlying problem in compiletest (namely #81).

//~| expected type `std::boxed::Box<Foo + std::marker::Send + 'static>`
//~| found type `std::boxed::Box<Foo + 'static>`
//~| expected type `std::boxed::Box<(dyn Foo + std::marker::Send + 'static)>`
//~| found type `std::boxed::Box<(dyn Foo + 'static)>`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This fix seems simple, but I don't really understand why this is the fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just what the latest nighty rustc outputs... they probably changed the text of the message.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, interesting... That means that even if you write Box<Trait> the error message is going to write Box<dyn Trait> (or is it Box<(dyn Trait)>?). I wonder if that's intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That looks strange, indeed... I assumed that here we are testing that compiletest matches errors correctly, not that rustc produces the correct message.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be honest, I just copied some tests from rustc's test suite when developing the test project - it was just to verify that things worked as expected, and for use as a future benchmark.

I think it's a result of the "dyn everywhere" approach: rust-lang/rust@4b18085. In particular: https://github.com/rust-lang/rust/blob/master/src/test/ui/mismatched_types/trait-bounds-cant-coerce.stderr.

@laumann
Copy link
Collaborator

laumann commented Jul 27, 2018

Nice, 🤞 that the tests work. Thank you so much for doing this!

Once this is merged I'll put out a new version.

src/runtest.rs Outdated
@@ -211,8 +211,12 @@ impl<'test> TestCx<'test> {

fn check_correct_failure_status(&self, proc_res: &ProcRes) {
// The value the rust runtime returns on failure
const RUST_ERR: i32 = 1;
if proc_res.status.code() != Some(RUST_ERR) {
let rust_err: i32 = if cfg!(feature = "stable") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was afraid of this. The whole "support stable Rust" is starting to become unmaintainable :-/ The check_correct_failure_status() function is somewhat different in rustc/compiletest so this fix isn't that back-portable.

I suppose it's fine for now, but I think a bigger sync-up with the current rustc/compiletest would be a good idea. I just worry that it would stable support.

Would it be better to skip the tests for the stable feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, thanks, I've also filed #121. Once this passes, we should be good to go. Thanks again!

@laumann laumann merged commit 502a85e into Manishearth:master Jul 27, 2018
@laumann
Copy link
Collaborator

laumann commented Jul 27, 2018

0.3.13 is out!

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.

Have AppVeyor build run tests in test-project
2 participants