Re: [PATCH] gdb/testsuite: modernize gdb.base/maint.exp

[Pedro Alves <pedro@palves.net>]

Hi Keith,

On 2022-06-24 18:51, Keith Seitz wrote:
> On 6/24/22 08:22, Pedro Alves wrote:
>> On 2022-06-21 16:52, Andrew Burgess via Gdb-patches wrote:
>>>> +                pass "$test_name 1"
>>> You could use:
>>>
>>>      pass "$gdb_test_name, pattern 1"
>>>
>>> here, and similar, with ', pattern 2' for the next 'pass' call.
>>>
>>
>> How about
>>
>>     pass "$gdb_test_name (pattern 1)"
>>
>>     pass "$gdb_test_name (pattern 2)"
>>
>> ?
>>
>> The idea being that the text in the trail parens is not considered part of the
>> test name, so when comparing gdb.sum files and matching test names, that parens part
>> should be discarded.  Whether this test passes with pattern 1 or 2 should make
>> no difference IIUC, thus I think it should not be part of the (part that counts
>> as real) test name.
>>
> I think it no surprise that I disdain this " (whatever)" idiom in test names. There
> is also a long-standing guideline in the wiki against this:
> 
> https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages
> 
> Has there been some unannounced, sekrit change to this policy?

There has not.

As mentioned in the wiki, the idea of not putting trailing " (foo)" in the test names,
is that timeouts, xfail, kfail, internal error, etc. info is appended to the test
name in gdb.sum.  So this means that tools that compare test results and track
regressions/progressions at the individual test level should consider these the same test:

 PASS: check foo thing
 FAIL: check foo thing (timeout)
 FAIL: check foo thing (GDB internal error)
 KFAIL: check foo thing (PRMS: gdb/99999)
 (etc.)

So what you shouldn't do is use trailing parens to de-DUPLICATE test names.  Like,
this is wrong:

 gdb_test "foo cmd" " = 1" "foo cmd (1)"
 gdb_test "foo cmd" " = 2" "foo cmd (2)"

because the test analysis tooling should consider that those two tests have the
same name, thus, they are duplicates.  The duplication-detection machinery we
have today directly in the testsuite doesn't flag that, but I think it should.

You should thus write instead something like:

 gdb_test "foo cmd" " = 1" "foo cmd, 1st"
 gdb_test "foo cmd" " = 2" "foo cmd, 2nd"

Now, the case at hand is different -- we have a single test, with multiple
regexps, like:

 gdb_test_multiple "foo cmd" "some test name" {
    -re -wrap "pattern 1" {
       # machine/run 1 hits this one.
       pass "$gdb_test_name - pattern 1"
    }
    -re -wrap "pattern 2" {
       # machine/run 2 hits this one.
       pass "$gdb_test_name - pattern 2"
    }
 }

I think that's wrong, because depending on whatever pattern you get, you get
a different test name, making it harder to automatically compare test results.
Depending on machines or testsuite runs, you get one of these:

   PASS: some test name - pattern 1
   PASS: some test name - pattern 2
   FAIL: some test name (timeout)
   FAIL: some test name (GDB internal error)

By making it instead:

 gdb_test_multiple "foo cmd" "some test name" {
    -re -wrap "pattern 1" {
       # machine 1 hits this one.
       pass "$gdb_test_name (pattern 1)"
    }
    -re -wrap "pattern 2" {
       # machine 2 hits this one.
       pass "$gdb_test_name (pattern 2)"
    }
 }

and given the rule that trailing (foo) doesn't count, all the pass
calls above, and the pass/fail calls inside gdb_test_multiple have
the same test message.

It's effectively the same as:

gdb_test_multiple "foo cmd" "some test name" {
   -re -wrap "pattern 1" {
      # machine 1 hits this one.
      pass $gdb_test_name
   }
   -re -wrap "pattern 2" {
      # machine 2 hits this one.
      pass $gdb_test_name
   }
}

An alternative is to really write "pass $gdb_test_name", and precede it with
'verbose -log "got pattern 1"' or some such.  I'm fine with that, but given the fact
that tooling should already be ignoring the trailing parens, I don't think we need
to stop using trailing parens in scenarios like these.

> 
> How is, e.g., "- pattern 1" any less desirable/informative than "(pattern 1)"?

I hope I've clarified it above.