Make UI test annotations mandatory

[GuillaumeGomez]

Follow-up of #11249.

I'm currently stuck with some errors about "substrings not found" whereas they are most definitely in the output. Example with test/ui/unwrap_or.rs:

error: substring `try: `unwrap_or_else(|| "Fail".to_string())`` not found in stderr output
 --> tests/ui/unwrap_or.rs:8:16
  |
8 |     //~| HELP: try: `unwrap_or_else(|| "Fail".to_string())`
  |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected because of this pattern
  |

error: substring `try: `unwrap_or_else(|| "Fail".to_string())`` not found in stderr output
  --> tests/ui/unwrap_or.rs:14:16
   |
14 |     //~| HELP: try: `unwrap_or_else(|| "Fail".to_string())`
   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected because of this pattern
   |

error: There were 1 unmatched diagnostics
 --> tests/ui/unwrap_or.rs:5:47
  |
5 |     let s = Some(String::from("test string")).unwrap_or("Fail".to_string()).len();
  |                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Help: try
  |

error: There were 1 unmatched diagnostics
  --> tests/ui/unwrap_or.rs:12:47
   |
12 |     let s = Some(String::from("test string")).unwrap_or("Fail".to_string()).len();
   |                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Help: try
   |

As you can see, I get both the error that there is no such substring and that I didn't match the help. Did I miss something obvious? Maybe you know @Centri3 ?

changelog: none

[rustbot]

r? @Manishearth

(rustbot has picked a reviewer for you, use r? to override)

[Manishearth]

I am opposed to this merging with each test having the full text of the error in it. This leads to a lot more test churn. I think this work needs to be done carefully, looking at each test and seeing what stuff should be in the test file and what stuff should just go in the ui output file.

cc @rust-lang/clippy we should probably discuss this as a team

Posted more context on zulip)

[bors]

☔ The latest upstream changes (presumably #11418) made this pull request unmergeable. Please resolve the merge conflicts.

[GuillaumeGomez]

So from the discussion on zulip, it was decided that only errors and the clippy lint name should be generated. But errors should still be present in UI test files.

[Manishearth]

it was decided that only errors and the clippy lint name should be generated. But errors should still be present in UI test files

I don't know what you mean by this, can you link to the actual conclusion?

[GuillaumeGomez]

Sorry, forgot to add it. It's here.

[Manishearth]

Ah. Right, I remember that. The proposal is to autogenerate annotations that have the lint name, only for errors, and then mandate them. Sure.

[GuillaumeGomez]

Exactly. I'll need to update ui_test a little bit to allow to have other annotations than ERROR but not making them mandatory for all cases (ie, if you add a HELP annotation on one error, to not make it mandatory for all errors in this file).

[Alexendoo]

There's oli-obk/ui_test#165 for matching lint names

[GuillaumeGomez]

Ah nice. But that's not the only thing needed. I'm working on something else needed too which I described in my previous comment.

[Alexendoo]

My impression from the meeting was that we don't plan to have NOTE/HELP annotations in which case ui_tests current behaviour seems fine (I didn't realise at the time that they were only required if already present in the file)

[GuillaumeGomez]

Yes but we need to take into account that some existing UI tests have help/note annotations.

[GuillaumeGomez]

So this PR is now waiting for oli-obk/ui_test#165 and for oli-obk/ui_test#182 to be merged.

EDIT: oli-obk/ui_test#182 allows to not change the minimum annotation level (in our case, we don't want to make annotations below "error" level mandatory) and oli-obk/ui_test#165 allows to set the lint name as annotation message instead of the actual message.

[Alexendoo]

I would imagine that we will mass replace the existing patterns with oli-obk/ui_test#165 style ones before making them mandatory, at which point we can also remove the NOTE annotations

[GuillaumeGomez]

Why removing them? They're already here so better keep them no?

[Alexendoo]

People look at existing test files for inspiration when writing their own so we'd want to avoid having multiple different styles across the test suite

[xFrednet]

Hey @GuillaumeGomez, this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation?

If you have any questions, you're always welcome to ask them in this PR or on Zulip.

@rustbot author

[GuillaumeGomez]

Yes I am. Did the new version of ui_test came out with the needed feature by any chance? (in rust nation with limited internet access ^^')

[xFrednet]

I don't know, can you maybe check once you're back home? If not we can ask in the repo and see what needs to be done to move this forward :D

[GuillaumeGomez]

I'll check when I'm back then and publish the update here once done. 👍

[xFrednet]

Hey @GuillaumeGomez, this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation?

If you have any questions, you're always welcome to ask them in this PR or on Zulip.

@rustbot author

[GuillaumeGomez]

Ah right, need to check if the update was done. Thanks for the ping!

[bors]

☔ The latest upstream changes (presumably #13098) made this pull request unmergeable. Please resolve the merge conflicts.

[GuillaumeGomez]

Still failing due to this error (in multiple files):

thread 'main' panicked at /home/imperio/.cargo/registry/src/index.crates.io-6f17d22bba15001f/annotate-snippets-0.11.4/src/renderer/display_list.rs:1103:29:
byte index 18 is not a char boundary; it is inside '脑' (bytes 16..19) of `    let _ = "电脑\0".as_ptr();`
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[bors]

☔ The latest upstream changes (presumably #13143) made this pull request unmergeable. Please resolve the merge conflicts.

[Alexendoo]

Opened oli-obk/ui_test#250

Adding the annotation would get rid of the panic I believe, we don't get that many new tests containing multibyte chars so it may be fine to ignore it for now. Or we can wait until it's fixed of course

[GuillaumeGomez]

@oli-obk is already aware of it. We wrote a small reproducer and they're working on a fix. So now we just need to wait. :)

[oli-obk]

ui_test 0.25 has been released and fixes this issue. There should be no API incompatibilities from the clippy side, but I still needed to do a major bump because it changes other public APIs

[flip1995]

rust-clippy/tests/compile-test.rs

Lines 209 to 217 in 37f4fbb

	ui_test::run_tests_generic(
	vec![config],
	ui_test::default_file_filter,
	|config, path, _file_contents| {
	config
	.program
	.envs
	.push(("CLIPPY_CONF_DIR".into(), Some(path.parent().unwrap().into())));
	},

@oli-obk I just tried updating ui_test. In the per_file_config closure of the run_test_generic, we've been using the test file path. But the closure no longer receives this path. Not sure if we can work around this.

[oli-obk]

The path is now in the span of the file_contents

[bors]

☔ The latest upstream changes (presumably #13126) made this pull request unmergeable. Please resolve the merge conflicts.