Move compile_fail doctests to ui compiletests. #477
Conversation
|
There are some known issues with running |
|
I recalled that the issue last time was that everyone develops on Macs so not being able to test on OSX was annoying. I believe I've fixed the issue by manually configuring the linking path for OSX. |
|
I am not sure if I understand the advantages of doing things this way. I like these as doctests for 2 primary reasons:
Could you clarify more about what advantages you see here? As it stands now I don't think I'd be in favor of merging this |
|
Currently, if a change in our code causes these examples to not compile for a reason other than what we are checking for, this fails silently. In nightly it is possible to specify an error code to assert for doc tests, but this is not likely to stabilize soon. This approach is how the compiler team asserts that code does not compile for the reason that they are expecting. If we want to expand the state docs to reiterate these rules, I would support that, but I do not think we need such verbose examples. If it's critical that it runs on ARM, then I can spend some time getting that to work (I do not think it's an actual issue, just that using --target changes where some dependencies are found). I would really prefer that we move to this because currently I have to periodically I uncomment |
| @@ -88,6 +88,11 @@ jobs: | |||
| make CC=aarch64-linux-gnu-gcc | |||
| sudo make install | |||
| rustup target add aarch64-unknown-linux-gnu | |||
| - name: UI Test | |||
There was a problem hiding this comment.
I find this terminology a bit disconcerting. I think the use in the referenced article refers to the technique being used to test the Rust compiler UI, but that isn't the case here. Maybe "type safety tests" or "type correctness tests"?
| @@ -10,6 +10,9 @@ build +FLAGS='': | |||
| lint-docs: | |||
| markdownlint '**/*.md' -c .markdownlint.json | |||
|
|
|||
| ui-test +FLAGS='': | |||
There was a problem hiding this comment.
Again consider renaming this
| #[test] | ||
| fn compile_test() { | ||
| let mut config = compiletest_rs::Config::default(); | ||
| config.mode = compiletest_rs::common::Mode::Ui; |
There was a problem hiding this comment.
Hmm, okay, maybe UI really is an idiomatic term for this. I'm surprised but if that's what Rust folks use then ignore previous comments.
Yeah, it is idiomatic and specifies the type of test we are running. It is also not the name I would have chosen. FWIW we are comparing the exact compiler output here, so in effect we are testing the compiler's UI rather than for a specific error code or something. If the UI changes we will have to update the stderr files, but hopefully that will be infrequent. @thomastaylor312 I know you had some concerns, am I good to merge? |
|
Seeing as @itowlson has LGTM'd this, you are good to merge whenever |
Moves the four
compile_faildoctests fromkubelet::stateto UI tests using compiletest-rs. This ensures that the compilation is failing for exactly the reason we expect (the stderr is compared).