Stream: t-compiler

Topic: ui test comments


mark-i-m (Mar 03 2020 at 19:57, on Zulip):

Why do we have the comments in the .rs file (//~ERROR ...)? They are pretty annoying to change and they nullify the benefits of bless. They seem somewhat redundant to me. Am I missing something?

RalfJ (Mar 03 2020 at 20:08, on Zulip):

they ensure that we actually get errors where errors should be

RalfJ (Mar 03 2020 at 20:08, on Zulip):

when I write a compile-fail test that expects errors in 5 places, I want to be sure that someone dosnt use --bless on it until there is just 1 error left

RalfJ (Mar 03 2020 at 20:08, on Zulip):

sometimes I also want to be sure that from multiple things that could error in that line, I get a specific error

nikomatsakis (Mar 03 2020 at 20:31, on Zulip):

indeed, at some point we weren't requiring them for UI tests, and it was all too easy to overlook changes that were unexpected

pnkfelix (Mar 04 2020 at 02:15, on Zulip):

@mark-i-m they aren't as annoying as they used to be, since they no longer have to carry as much of the diagnostic message for their utility. Many people now just leave just the error code on them.

pnkfelix (Mar 04 2020 at 02:15, on Zulip):

(and leave the diagnostic details to the .stderr file)

centril (Mar 04 2020 at 02:46, on Zulip):

It's not a good idea to use error codes in //~ ERROR comments, as error codes are not really stable or meaningful

pnkfelix (Mar 04 2020 at 14:29, on Zulip):

To be honest, I prefer to include messages (as you imply is better practice), since they are indeed self-documenting and thus inherently more meaningful

pnkfelix (Mar 04 2020 at 14:30, on Zulip):

but I don't understand the basis with which you say that error codes are not stable.

centril (Mar 04 2020 at 22:45, on Zulip):

Error codes can be reused and whatnot; I don't think we have a rule that says "once you've used an error code for something you may never reuse it" -- but the main point is that they are not very meaningful when reading tests and whatnot

centril (Mar 04 2020 at 22:46, on Zulip):

if we had more semantic error codes instead of numbers I think the story might be different

mark-i-m (Mar 05 2020 at 00:46, on Zulip):

@pnkfelix Hmm... so is it acceptable to truncate an existing test's comments? In particular, I have 97 failing tests on one of my branches because a word in an error changed (but it's still the same error).

mark-i-m (Mar 05 2020 at 00:46, on Zulip):

Oh, whoops Zulip was hiding the rest of this thread... reading it now

mark-i-m (Mar 05 2020 at 00:49, on Zulip):

Hmm... I see

mark-i-m (Mar 05 2020 at 00:51, on Zulip):

What if we made compiletest only check that the string in the error appears somewhere in the error message? Then, we could write much short comments that are less likely to go stale (and the full .stderr records the whole message).

centril (Mar 05 2020 at 01:01, on Zulip):

@mark-i-m I think it's acceptable to not include the whole string, assuming it includes some important parts

mark-i-m (Mar 05 2020 at 01:02, on Zulip):

@centril Do you mean that that's possible today or that you would be ok with us making it that way? (I'm aware that compiletest allows a prefix of an error, but does it allow arbitrary substrings?)

lqd (Mar 05 2020 at 01:02, on Zulip):

IIRC it's possible today (but not an arbitrary substring :)

centril (Mar 05 2020 at 01:02, on Zulip):

@mark-i-m it's possible today to use a prefix

centril (Mar 05 2020 at 01:03, on Zulip):

I feel a prefix is the right balance

lqd (Mar 05 2020 at 01:04, on Zulip):

do you feel arbitrary substrings would be less likely to go stale than prefixes ?

centril (Mar 05 2020 at 01:05, on Zulip):

(I assume it's not a question directed towards me)

mark-i-m (Mar 05 2020 at 01:06, on Zulip):

do you feel arbitrary substrings would be less likely to go stale than prefixes ?

That's a great question

mark-i-m (Mar 05 2020 at 01:06, on Zulip):

/me looks at my PR again

lqd (Mar 05 2020 at 01:08, on Zulip):

@centril indeed :) I don't mind prefixes either, but if arbitrary substrings somehow managed to be better, maybe it could be worthwhile; I'm not sure diagnostics changes happen more likely in the beginning rather than the rest of the message

centril (Mar 05 2020 at 01:08, on Zulip):

perhaps we should have a length restriction on the substring

lqd (Mar 05 2020 at 01:09, on Zulip):

e.g. the most recent PR where that happened to me was changes in the middle of the message

centril (Mar 05 2020 at 01:09, on Zulip):

"substring cannot be shorter than X characters"

mark-i-m (Mar 05 2020 at 01:35, on Zulip):

Another option of course is to not check the actual sting in the comment. That is, we only check that the right kind of diagnostic is emitted there, and the rest of the comment is purely documentation

mark-i-m (Mar 05 2020 at 01:36, on Zulip):

Looking at my PR again, I don't arbitrary substrings would really help, because in my case I was changing a key word in the error

oli (Mar 05 2020 at 09:24, on Zulip):

mark-i-m said:

Another option of course is to not check the actual sting in the comment. That is, we only check that the right kind of diagnostic is emitted there, and the rest of the comment is purely documentation

that works already I think? You can just specify //~ ERROR without any info

centril (Mar 05 2020 at 10:19, on Zulip):

(Please don't do that though)

pnkfelix (Mar 05 2020 at 15:01, on Zulip):

centril said:

Error codes can be reused and whatnot; I don't think we have a rule that says "once you've used an error code for something you may never reuse it" -- but the main point is that they are not very meaningful when reading tests and whatnot

I think there was explicit discussion in the past that we did not want to reuse error codes. The main reasoning was that error codes are a common thing that comes up when one uses a search engine to try to better understand an error message.

pnkfelix (Mar 05 2020 at 15:02, on Zulip):

my brief skims of my email chains and of internals.rlo posts did not bring up any evidence supporting my claim above, however.

pnkfelix (Mar 05 2020 at 15:03, on Zulip):

anyway that is why I said I was confused by @centril 's statement that error codes were not stable. Because, to my mind, they are.

nikomatsakis (Mar 05 2020 at 15:06, on Zulip):

We don't generally re-use error codes and, even if we did, I don't think it's likely to cause any real problems.

nikomatsakis (Mar 05 2020 at 15:06, on Zulip):

Personally I thikn substring would likely be ok.

nikomatsakis (Mar 05 2020 at 15:07, on Zulip):

The ERROR lines are basically a heuristic to try and ensure that things don't change "too much" or in unexpected ways

nikomatsakis (Mar 05 2020 at 15:07, on Zulip):

I've sometimes wanted regex too :)

nikomatsakis (Mar 05 2020 at 15:07, on Zulip):

but anyway I definitely feel like that using some prefix is just fine, and I think I agree that //~ ERROR by itself is probably too little...

nikomatsakis (Mar 05 2020 at 15:07, on Zulip):

a "distinctive substring" seems fine though

nikomatsakis (Mar 05 2020 at 15:09, on Zulip):

nikomatsakis said:

We don't generally re-use error codes and, even if we did, I don't think it's likely to cause any real problems.

to elaborate: you'd have to (a) deprecate the ERROR code and (b) replace it with something semantically different and (c) have some test that unexpected creates this new error on the same line as before, all in one PR

nikomatsakis (Mar 05 2020 at 15:09, on Zulip):

that said, I used to do the E123 codes, but I've started to avoid them because (as @pnkfelix wrote) they're less self documenting

mark-i-m (Mar 06 2020 at 23:06, on Zulip):

I think compiletest actually already does only substring checking:

// src/tools/compiletest/src/runtest.rs:1375
                expected_errors.iter().enumerate().position(|(index, expected_error)| {
                    !found[index]
                        && actual_error.line_num == expected_error.line_num
                        && (expected_error.kind.is_none()
                            || actual_error.kind == expected_error.kind)
                        && actual_error.msg.contains(&expected_error.msg) // <------ substring of error message
                });
mark-i-m (Mar 06 2020 at 23:06, on Zulip):

So if there is consensus, it seems that "distinctive substring" is the direction we want to go?

RalfJ (Mar 08 2020 at 09:29, on Zulip):

I think that describes what I try to do, yes

Last update: Jun 04 2020 at 18:20UTC