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

Add compile-fail and compile-pass tests #17

Merged
merged 15 commits into from
Jul 25, 2018
Merged

Add compile-fail and compile-pass tests #17

merged 15 commits into from
Jul 25, 2018

Conversation

LukasKalbertodt
Copy link
Member

Hi @KodrAus,

I'd like to hear your opinion about this change :). See "questions" at the end.

Sorry for the long text, but it's necessary ...

... Story time!

(don't worry, there is a summary at the end)

So in order to tackle #15, I thought about having compile-fail tests. There is the crate compiletest-rs which is basically the extracted test utility from rustc which also offers compile-fail tests. However, I decided not to use that for several reasons. The code of said crate is really old and messy, since it lived in the rustc repo for quite some time already. The API is really not nice. Additionally, it offers far more than compile-fail tests ... which we don't need. Many crates do use that crate to do compile-fail tests, but I don't think that's the way to go.

So I started writing my own little script (it works by adding a harness = false test). While writing that, I noticed that I tried to make the script look and behave like the default test harness (with the familiar testing output). This seemed like something that can be extracted into a crate to avoid everyone writing the same code over and over again. That's why I wrote libtest-mimic which help to mimic the output and behavior of the standard libtest. I have to say that I'm pretty happy how it turned out.

My crate is not yet on crates.io and is not considered stable yet! That's why this PR is still marked as WIP.

One problem occurred while writing the compile-fail script. To compile all tests I have to call rustc. But I also need to pass a proper --extern auto_impl=path-to-auto-impl-artifact flag. The path is something like /target/debug/deps/libauto_impl-9ba2e80e05cf7099.so ... so not something one could guess. So this is tricky. There are several possibilities:

  • One could write tiny Cargo projects on the disk during testing, copying the tests into the project and executing cargo build. Apart from the fact that writing stuff to disk during testing is a bit ugly, cargo has a bunch of output we don't really want.
  • One could somehow try to calculate the --extern path on your own, but that's hard if not impossible.
  • One can ask cargo. Either by executing cargo build -v and parsing that output OR, much better, by using Cargo's build-plans. Turns out, Cargo has a --build-plan flag which prints the plan of tasks to do in JSON format instead of doing something. This output contains the path we need.

I decided to go with the last possibility. But it has a disadvantage: build-plans are still unstable (tracking issue). This means that these compile-fail tests can only be executed on nightly! This might not yet be a problem, since the whole crate requires nightly, but it might be annoying in the future. Also: changes to the build plan could of course break the tests.

Through the build-plans tracking issue I found the build-plan crate which parses the build plan for me. That made it possible to remove a bunch of ugly JSON code.

Last part of my story: the build-plan crate is written by Jonas Schievink, someone who studies at the same university as I do and closely worked with me for my German "Programmieren in Rust" lecture. So that's always fun. While looking through his crates, I found compile-fail -- a crate that is an alternative to compiletest-rs!

I think it would be awesome to have a proper compile-fail test crate so that everyone (like auto_impl) can just use it. Sadly Jonas' crate is still WIP and... I disagree with a few decisions. But I'm talking to him right nowish and we'll see if we can polish that crate. Then I could use it here in auto_impl. I would also try to use my libtest-mimic crate in compile-fail.

Summary

This PR ...

  • ... adds a script for compile-fail tests, but it only work on nightly
  • ... adds five small compile-fail tests as examples
  • ... only checks that some code doesn't compile -- there is no comparison of the actual vs. the expected error message yet!
  • ... right now uses a git-dependency to a WIP library of mine
  • ... could potentially be improved in the future by using the compile-fail logic from another crate

cargo test now looks like this:

image

Questions

  • Do you think the general idea (i.e. "adding a [[test]] with harness = false) is good?
  • Do you think it's fine to merge my own compile-fail script for now? Or should we use an external compile-fail logic from the beginning?
  • Are you fine with using a crate that I wrote? I feel bad because it might look like I want to sneak dependencies to my crates into a bigger project to get moar $$$ downloads. That's of course not my intention.

If you think this is basically good to merge, I'd polish the libtest-mimic crate, publish it to crates.io and adjust this PR.


Well... this took longer than expected :P Writing this compile-fail script really lead me deep into the rabbit hole. Anyway: I think starting with good tests is really important for this crate.

Again, thank you for your time thinking about my questions! And maybe even reviewing the code changes, if you have that time.

@KodrAus
Copy link
Member

KodrAus commented Jul 22, 2018

Thanks for digging into this @LukasKalbertodt and awesome job putting it all together.

I'm pretty excited to see some more effort to rethink compile-fail tests, like you said they're a bit rough around the edges with compiletest-rs. I usually run into problems when integrating compiletest-rs that are solved by your consumption of the build plan rather than just poking around in output directories. So I think that's the best approach to take, and am ok with compile tests being stuck on nightly to make that work. It doesn't look like there are any blockers to stabilizing the build plans, so it's probably just waiting on folks like us to start using them to drive them towards stabilization.

Do you think the general idea (i.e. "adding a [[test]] with harness = false) is good?

This seems totally fine to me. As far as I know this is the sort of case that harness = false is meant to serve.

Do you think it's fine to merge my own compile-fail script for now?

Absolutely! I think working on the script using auto_impl as a first use-case could be a great way to build up another alternative approach to compile fail tests that's robust. Please also feel free to hoist whatever code ends up in here out into your own library at any point if you feel like it.

Are you fine with using a crate that I wrote?

Absolutely!

So I have no problem merging this in and we can work on inspecting the error messages in a follow-up. As a random aside, I've found matching error strings in output is pretty brittle and discourages (at least discourages me) from tuning error messages. What do you think about adding our own simple error codes to diagnostic messages that we can use to assert the presence of in compile tests?

@KodrAus
Copy link
Member

KodrAus commented Jul 22, 2018

@LukasKalbertodt would you like to work more in this PR or are you happy for me to drop the WIP and merge as-is since this already implements a reasonable and working testing strategy now, even though there's more we'd like to do?

@LukasKalbertodt
Copy link
Member Author

As a random aside, I've found matching error strings in output is pretty brittle and discourages (at least discourages me) from tuning error messages. What do you think about adding our own simple error codes to diagnostic messages that we can use to assert the presence of in compile tests?

That sounds like a good idea. I'm also not sure what to think of error message checks. I can see their benefits, but yeah, they make changing the error message difficult. I'm still puzzled how the Rust compiler switched to the new error format (one or two years ago? Can't quite remember). Some poor soul probably had to fix all ui tests ...

would you like to work more in this PR or are you happy for me to drop the WIP and merge as-is since this already implements a reasonable and working testing strategy now, even though there's more we'd like to do?

I'd just like to polish it before merging. In particular, my crate I'm using should be on crates.io and auto_impl shouldn't just link to a repo's master branch. I'll remove the WIP flag and ping you once it's ready!

@LukasKalbertodt
Copy link
Member Author

@KodrAus I pushed two commits. libtest-mimic is now on crates.io and this version is used here. Apart from that, it's only a minor change.

If you want to take a look again, go ahead and just merge it if it looks good to you. If you don't have time, that's no problem -- I will just merge it in the next couple of days (since you already said OK and the additional changes are small).

@LukasKalbertodt LukasKalbertodt changed the title WIP: Add compile-fail tests Add compile-fail tests Jul 23, 2018
@LukasKalbertodt LukasKalbertodt mentioned this pull request Jul 23, 2018
6 tasks
@LukasKalbertodt
Copy link
Member Author

@KodrAus I decided to include more changes in this PR.

First I thought that I would make all compile-pass tests (formerly in compile_test/) a separate file in tests/. However, I don't quite like that. Yes, it has the desired effect: if it doesn't compile, it fails. But each file is compiled as a test-suite and the output says "executed 0 tests".

So instead I decided to create a compile-pass script, just like compile-fail. Of course, the code used by both scripts (which was most code) is shared and lives in tests/util/. I also moved all tests in compile_test into separate files and removed the old folder. Now cargo test looks like this:

image

I hope you agree with those changes! If yes, you can merge, this is ready.

Removing the `script` value completely makes Travis execute a default
script for the language.
@LukasKalbertodt LukasKalbertodt changed the title Add compile-fail tests Add compile-fail and compile-pass tests Jul 23, 2018
@KodrAus
Copy link
Member

KodrAus commented Jul 25, 2018

This is a great improvement! Thanks @LukasKalbertodt

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.

2 participants