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

Use trybuild to do compile-fail/pass tests (and fix some minor bugs) #59

Merged
merged 4 commits into from
Nov 28, 2019

Conversation

LukasKalbertodt
Copy link
Member

Thanks again @CreepySkeleton for mentioning trybuild.

I'm not 100% happy with the crate:

  • It doesn't use an own test harness and it's not possible to filter for specific tests (I think)
  • One has to add a main function to every single test

But, it works easily and it compares the stderr which is something I wanted for a long time. And it's not super old as compiletest-rs. So I think it's a good idea to make this change, even if the crate is not perfect.

Fixes #55


Due to using proper compile-fail testing now, I noticed two bugs actually.

  • Our hack in diag.rs uses a thread local variable containing a Vec<_>. If our macro exits when the vector still contains elements, the Rust compiler panics. Not just that, it tries to panic but cannot, leading to a "fatal runtime error" and an abort. Uhg. I should probably create an official issue, though I think using thread locals was never officially allowed in proc macros. Workaround: just clear the vector before we are done.
  • We didn't check for #[auto_impl(...)] attributes on trait items other than methods. That's fixed now.

@CreepySkeleton
Copy link
Contributor

CreepySkeleton commented Oct 31, 2019

Our hack in diag.rs uses a thread local variable containing a Vec<_>. If our macro exits when the vector still contains elements, the Rust compiler panics. Not just that, it tries to panic but cannot, leading to a "fatal runtime error" and an abort. Uhg. I should probably create an official issue, though I think using thread locals was never officially allowed in proc macros. Workaround: just clear the vector before we are done.

Yep, I had to yank one version of my crate due to it 😆

src/diag.rs Outdated Show resolved Hide resolved
TraitItem::Const(c) => (&mut c.attrs, false),
TraitItem::Type(t) => (&mut t.attrs, false),
TraitItem::Macro(m) => (&mut m.attrs, false),
_ => panic!("encountered unexpected `TraitItem`, cannot handle that, sorry!"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Guess what will happen if this panic occurs after you emitted an error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch! However, it is clear that the error handling has to change in some way. Either by using your crate or in another way. But that's not something this PR will fix -- I just applied fixes so that nothing immediately breaks. In short: your comment will be addressed in some future PR.

@CreepySkeleton
Copy link
Contributor

And also there's a downside in trybuild - it officially supports only rustc +1.36, but there's a workaround

@KodrAus
Copy link
Member

KodrAus commented Oct 31, 2019

And also there's a downside in trybuild - it officially supports only rustc +1.36, but there's a workaround

Bumping our MSRV is something I’m comfortable with 👍

@LukasKalbertodt
Copy link
Member Author

Regarding MSRV: this is a dev-dependency, right? So users of this crate do not need to worry about it at all. The newer Rust version is just required for testing.

@LukasKalbertodt LukasKalbertodt force-pushed the use-trybuild branch 3 times, most recently from da3bd93 to 13664ba Compare October 31, 2019 12:08
@LukasKalbertodt
Copy link
Member Author

Oh, meh. So now we run into problems where stable/beta/nightly might have different error messages. So our stderr files need to target one of those three?

@CreepySkeleton
Copy link
Contributor

Regarding MSRV: this is a dev-dependency, right? So users of this crate do not need to worry about it at all. The newer Rust version is just required for testing.

Yes. But you won't even be able to run cargo test on pre-1.36 rustc because dev-dependencies cannot be optional.

Oh, meh. So now we run into problems where stable/beta/nightly might have different error messages. So our stderr files need to target one of those three?

The recommended approach here is to run tests only on one channel, presumably on stable to reduce maintenance burden (nightly messages tend to change for no good reason). Personally, I recommend rustversion for this:

#[rustversion::attr(not(stable), ignore)]
#[test]
fn ui() {
    let t = trybuild::TestCases::new();
    t.compile_fail("tests/ui/*.rs");
}

src/diag.rs Outdated Show resolved Hide resolved
This wasn't checked before actually. Currently the only attribute we
have is `keep_default_for`, so it only makes sense on methods.
This improves the previous situation where I had written some test
logic myself. The previous solution did not check stderr. Sadly,
`trybuild` requires a `main` function in every single test, leading
to a lot of changes in this commit.
@CreepySkeleton
Copy link
Contributor

@LukasKalbertodt should I base my PR on top of this one?

@LukasKalbertodt
Copy link
Member Author

@CreepySkeleton Yeah, I guess that's a good idea.

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.

Add proper compile-fail tests
3 participants