Re: [PATCH] gdb.btrace/*.exp: Make test names unique

[Simon Marchi <simon dot marchi at ericsson dot com>]

Hi Markus,

On 16-10-06 09:58 AM, Metzger, Markus T wrote:
>> This patch makes the btrace test names unique.  I was trying to
>> understand why rn-dl-bind.exp fails on my machine, and saw that it
>> failed at test "next".  Given that there are multiple "next" in that
>> test, it's hard to know which one doesn't work.
>>
>> In most cases, I simply added a "#N" suffix to the test cases, since I
>> couldn't find a more appropriate name from the context.
>>
>> For the record, I still don't know why rt-dl-bind.exp fails, but I don't

(I typo'ed here, it's rn-dl-bind.exp)

>> intend to spend more time on it (at least for now).
> 
> On what system are you seeing those fails?

Distro: Ubuntu 14.04.5 LTS
Kernel: 4.7.0 x86_64
CPU: Intel(R) Core(TM) i5-4310U CPU @ 2.00GHz

Here's the gdb.log, if it can help you.

http://paste.ubuntu.com/23284745/

>> diff --git a/gdb/testsuite/gdb.btrace/delta.exp
>> b/gdb/testsuite/gdb.btrace/delta.exp
>> index c9dbf38..c822400 100644
>> --- a/gdb/testsuite/gdb.btrace/delta.exp
>> +++ b/gdb/testsuite/gdb.btrace/delta.exp
>> @@ -47,7 +47,7 @@ with_test_prefix "no trace" {
>>  }
>>
>>  # we record each single-step, even if we have not seen a branch, yet.
>> -gdb_test "stepi"
>> +gdb_test "stepi" "main\.4.*" "stepi #1"
> 
> I wouldn't count on the stepi landing there.  This is also not relevant for the
> test.
> 
> I'd rather add test prefixes for the below groups of tests and leave the initial
> "next" and "stepi" that move to the relevant code and record the trace as
> they are.

Ok.  Could we test_prefix them with maybe "setup", so it's clear what they're for?

> I.e. here...
> 
>>  # check that we can reverse-stepi that instruction
> 
> ... and here ...
> 
>>  # and back
> 
> We also wouldn't need to count the "info record" commands in this case.

Right, so maybe prefix those blocks with "backwards" and "forward"?

>> diff --git a/gdb/testsuite/gdb.btrace/enable.exp
> 
> Instead of adding a counter here ...
> 
>>  # enable btrace
>> -gdb_test_no_output "record btrace" "record btrace"
>> +gdb_test_no_output "record btrace" "record btrace #1"
> 
> ... we might want to add a test prefix here ...

Perhaps "first record", "second record" and "record after re-run"?

>> @@ -90,7 +90,7 @@ clean_restart $testfile
>>  if ![runto_main] {
>>      return -1
>>  }
>> -gdb_test_no_output "record btrace"
>> +gdb_test_no_output "record btrace" "record btrace #2"
>>  if ![runto_main] {
>>      return -1
>>  }
> 
> 
>> diff --git a/gdb/testsuite/gdb.btrace/function_call_history.exp
>> b/gdb/testsuite/gdb.btrace/function_call_history.exp
>> index 7d1e4049..53fd239 100644
>> --- a/gdb/testsuite/gdb.btrace/function_call_history.exp
>> +++ b/gdb/testsuite/gdb.btrace/function_call_history.exp
>> @@ -30,7 +30,7 @@ if ![runto_main] {
>>  }
>>
>>  # start btrace
>> -gdb_test_no_output "record btrace"
>> +gdb_test_no_output "record btrace" "record btrace #1"
>>
>>  # set bp after increment loop and continue
>>  set bp_location [gdb_get_line_number "bp.1" $testfile.c]
>> @@ -236,7 +236,7 @@ gdb_continue_to_breakpoint "cont to fib.3"
>>  gdb_continue_to_breakpoint "cont to fib.4"
>>
>>  # start tracing
>> -gdb_test_no_output "record btrace"
>> +gdb_test_no_output "record btrace" "record btrace #2"
> 
> The bottom of this file from the second "runto_main" is really an
> independent test.
> 
> We might either prefix the entire test from the second runto_main until
> the end of the file or split the file into two.

If they test different aspects of the same feature, it's fine if they are in the same file.
I like to at least separate logically independent tests in procs:

  proc test_some_thing { } {
    ...
  }

  proc test_some_other_thing { } {
    ...
  }

  with_test_prefix "test some thing" { test_some_thing }
  with_test_prefix "test some other thing" { test_some_other_thing }

And as much as possible, have them be independent, meaning that one does not
rely on the state left by the other, so you can comment out all but one, reorder
them, and it would still work.  If you agree with that I can try to modify this
test so it looks like that.

>> diff --git a/gdb/testsuite/gdb.btrace/nohist.exp
> 
>> -proc check_not_replaying {} {
>> +proc check_not_replaying { test } {
>>    gdb_test "info record" [multi_line \
>>      "Active record target: record-btrace" \
>>      "Recording format: .*" \
>>  	"Recorded 0 instructions in 0 functions \\\(0 gaps\\\) for \[^\\\r\\\n\]*" \
>> -    ]
>> +    ] \
>> +    $test
>>  }
>>
>>  gdb_test_no_output "record btrace"
>>
>> -check_not_replaying
>> +check_not_replaying "check not replaying #1"
> 
> I think a couple of test prefixes would be more helpful, here.

Ok.

>> diff --git a/gdb/testsuite/gdb.btrace/non-stop.exp
> 
>> -with_test_prefix "navigate" {
>> +with_test_prefix "navigate #1" {
>>      gdb_test "thread apply 1 record goto 2" "$loop_line"
>>      gdb_test "thread apply 2 record goto 4" "$loop_line"
>>      gdb_test "thread apply 1 info record" \
>>          ".*Replay in progress\.  At instruction 2\."
>>      gdb_test "thread apply 2 info record" \
>>          ".*Replay in progress\.  At instruction 4\."
>> +}
>>
>> +with_test_prefix "navigate #2" {
>>      gdb_test "thread apply all record goto 5" "$loop_line"
>>      gdb_test "thread apply 1 info record" \
>>          ".*Replay in progress\.  At instruction 5\."
>> @@ -175,29 +181,37 @@ with_test_prefix "continue" {
>>      with_test_prefix "thread 1" {
>>          gdb_cont_to_no_history 1 "continue" 1
>>          gdb_test "thread apply 1 info record" \
>> -            ".*Recorded \[0-9\]+ instructions \[^\\\r\\\n\]*"
>> +            ".*Recorded \[0-9\]+ instructions \[^\\\r\\\n\]*" \
>> +            "thread apply 1 info record #1"
>>          gdb_test "thread apply 2 info record" \
>> -            ".*Replay in progress\.  At instruction 5\."
>> +            ".*Replay in progress\.  At instruction 5\." \
>> +            "thread apply 2 info record #1"
>>
>>          gdb_cont_to_no_history 1 "reverse-continue" 1
>>          gdb_test "thread apply 1 info record" \
>> -            ".*Replay in progress\.  At instruction 1\."
>> +            ".*Replay in progress\.  At instruction 1\." \
>> +            "thread apply 1 info record #2"
>>          gdb_test "thread apply 2 info record" \
>> -            ".*Replay in progress\.  At instruction 5\."
>> +            ".*Replay in progress\.  At instruction 5\." \
>> +            "thread apply 2 info record #2"
>>      }
> 
> Could we do the same trick you did with the "navigate" tests here, as well?
> I.e. split the "continue" tests into sub-groups with separate prefixes.

Do you mean:

with_test_prefix "thread 1" {
  with_test_prefix "continue forward" {
    gdb_cont_to_no_history 1 "continue" 1
    ...
  }

  with_test_prefix "continue backward" {
    gdb_cont_to_no_history 1 "reverse-continue" 1
    ...
  }
}

?

> 
>> diff --git a/gdb/testsuite/gdb.btrace/stepi.exp
>> b/gdb/testsuite/gdb.btrace/stepi.exp
>> index b21e4e5..54cde46 100644
>> --- a/gdb/testsuite/gdb.btrace/stepi.exp
>> +++ b/gdb/testsuite/gdb.btrace/stepi.exp
>> @@ -67,18 +67,18 @@ gdb_test_no_output "record btrace"
>>  gdb_test "next"
>>
>>  # we start with stepping to make sure that the trace is fetched automatically
>> -gdb_test "reverse-stepi" ".*fun4\.5.*"
>> -gdb_test "reverse-stepi" ".*fun4\.5.*"
>> +gdb_test "reverse-stepi" ".*fun4\.5.*" "reverse-stepi #1"
>> +gdb_test "reverse-stepi" ".*fun4\.5.*" "reverse-stepi #2"
>>
>>  # let's check where we are in the trace
>>  with_test_prefix "reverse-stepi to 39" { check_replay_at 39 }
>>
>>  # let's step forward and check again
>> -gdb_test "stepi" ".*fun4\.5.*"
>> +gdb_test "stepi" ".*fun4\.5.*" "stepi #1"
>>  with_test_prefix "stepi to 40" { check_replay_at 40 }
> 
> We could extend the test prefix to cover the stepping command in addition
> to the check.  We'd still need to count in a few cases where we need two
> identical stepping commands.

I can try that.

Thanks!

Simon