Re: [PATCH 1/2] Refactor gdb.trace/save-trace.exp

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

On 15-11-18 12:38 PM, Pedro Alves wrote:
> On 11/16/2015 06:44 PM, Simon Marchi wrote:
>> Some code is duplicated, to run the test twice with absolute and
>> relative paths, so I factored it out in a few procs.  It uses
>> with_test_prefix to differentiate between test runs.
> 
> You also made each run generate a separate .tr file, which is good too.

Well, it was still separate, as the runs generate the files in two different
directories:

./testsuite/gdb.trace/savetrace.tr
./testsuite/savetrace.tr

Now it's

./testsuite/gdb.trace/savetrace-absolute.tr
./testsuite/savetrace-relative.tr

But yes, at least the names are more explicit.

>> +proc gdb_save_tracepoints { save_path } {
> 
> Could you add an intro comment, mentioning what the parameter is for?

Done.

>> +    set save_path_regexp [string_to_regexp $save_path]
>> +    remote_file host delete $save_path
>> +    gdb_test "save tracepoints $save_path" "Saved to file '$save_path_regexp'." "save tracepoint definitions"
> 
> Could you break overly long lines (throughout) with \, like done elsewhere in the file?

Done.

>> +}
>> +
>> +proc gdb_load_tracepoints { save_path } {
> 
> Ditto.

Done.

Pushed, thanks!


>From 045ccf910b4345a1cfa9f3d3af20ae4d8d1defa2 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Mon, 23 Nov 2015 18:47:08 -0500
Subject: [PATCH] Refactor gdb.trace/save-trace.exp

Some code is duplicated, to run the test twice with absolute and
relative paths, so I factored it out in a few procs.  It uses
with_test_prefix to differentiate between test runs.

I replaced usages of "save-tracepoints" with "save tracepoint", since
the former is deprecated.

I also removed the "10.x", as it doesn't make much sense anymore.  It
isn't used in general in the testsuite, and I don't think it's really
useful.

gdb/testsuite/ChangeLog:

	* save-trace.exp: Factor out code to these...
	(gdb_save_tracepoints): New.
	(gdb_load_tracepoints): New.
	(do_save_load_test): New.
---
 gdb/testsuite/ChangeLog                |  7 +++
 gdb/testsuite/gdb.trace/save-trace.exp | 90 ++++++++++++++++++----------------
 2 files changed, 54 insertions(+), 43 deletions(-)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index e5c2b97..ffc7dd5 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2015-11-23  Simon Marchi  <simon.marchi@ericsson.com>
+
+	* save-trace.exp: Factor out code to these...
+	(gdb_save_tracepoints): New.
+	(gdb_load_tracepoints): New.
+	(do_save_load_test): New.
+
 2015-11-23  Kevin Buettner  <kevinb@redhat.com>

 	* gdb.base/asmlabel.exp: New test.
diff --git a/gdb/testsuite/gdb.trace/save-trace.exp b/gdb/testsuite/gdb.trace/save-trace.exp
index ab64fd0..aade1ed 100644
--- a/gdb/testsuite/gdb.trace/save-trace.exp
+++ b/gdb/testsuite/gdb.trace/save-trace.exp
@@ -63,12 +63,12 @@ foreach x { 1 2 3 4 5 6 } {
     set trcpt$x $trcpt
     gdb_test "passcount $x" \
 	     "Setting tracepoint $trcpt.* to $x" \
-	     "10.x: set passcount for tracepoint $trcpt"
+	     "set passcount for tracepoint $trcpt"

     gdb_test_no_output "condition $trcpt $x - 1 == $x / 2" \
-	     "10.x: set condition for tracepoint $trcpt"
+	     "set condition for tracepoint $trcpt"

-    gdb_trace_setactions "10.x: set actions for tracepoint $x" \
+    gdb_trace_setactions "set actions for tracepoint $x" \
 	    "" \
 	    "collect q$x" "^$" \
 	    "while-stepping $x" "^$" \
@@ -77,7 +77,28 @@ foreach x { 1 2 3 4 5 6 } {
 }

 gdb_test_no_output "set default-collect gdb_char_test, gdb_long_test - 100" \
-    "10: set default-collect"
+    "set default-collect"
+
+# Save tracepoint definitions to a file, at path SAVE_PATH.
+proc gdb_save_tracepoints { save_path } {
+    set save_path_regexp [string_to_regexp $save_path]
+    remote_file host delete $save_path
+    gdb_test "save tracepoints $save_path" \
+	     "Saved to file '$save_path_regexp'." \
+	     "save tracepoint definitions"
+}
+
+# Load tracepoint definitions from a file, from path SAVE_PATH.
+proc gdb_load_tracepoints { save_path } {
+    # Cleanup existing tracepoints/collections
+    gdb_delete_tracepoints
+    gdb_test_no_output "set default-collect" "clear default-collect"
+
+    gdb_test "info tracepoints" "No tracepoints." "delete tracepoints"
+
+    gdb_test "source $save_path" "Tracepoint \[0-9\]+ at .*" \
+	     "read back saved tracepoints"
+}

 proc gdb_verify_tracepoints { testname } {
     global gdb_prompt
@@ -109,55 +130,38 @@ proc gdb_verify_tracepoints { testname } {

     gdb_test "show default-collect" \
 	"The list of expressions to collect by default is \"gdb_char_test, gdb_long_test - 100\"..*" \
-	"10: show default-collect"
+	"verify default-collect"
 }

-gdb_verify_tracepoints "10.x: verify trace setup"
+proc do_save_load_test { save_path } {
+    # Save current tracepoint definitions to a file
+    gdb_save_tracepoints $save_path

-# 10.1 Save current tracepoint definitions to a file
+    # Clear existing tracepoints and reload from file
+    gdb_load_tracepoints $save_path

-remote_file host delete savetrace.tr
-gdb_test "save-tracepoints savetrace.tr" \
-	"Saved to file 'savetrace.tr'." \
-	"10.1: save tracepoint definitions"
+    # Check if they match the expected tracepoints
+    gdb_verify_tracepoints "verify recovered tracepoints"
+}

-# 10.2 Read back tracepoint definitions
+gdb_verify_tracepoints "verify trace setup"

-gdb_delete_tracepoints
-gdb_test_no_output "set default-collect" "10.2: clear default-collect"
-gdb_test "info tracepoints" "No tracepoints." "10.2: delete tracepoints"
-gdb_test "source savetrace.tr" \
-	"Tracepoint \[0-9\]+ at .*" \
-	"10.2: read back saved tracepoints"
-gdb_verify_tracepoints "10.2: verify recovered tracepoints"
-remote_file host delete savetrace.tr
-
-# 10.3 repeat with a path to the file
-
-set trace_file_name [standard_output_file savetrace.tr]
-set escapedfilename [string_to_regexp $trace_file_name]
-remote_file host delete $trace_file_name
-gdb_test "save-tracepoints $trace_file_name" \
-	"Saved to file '${escapedfilename}'." \
-	"10.3: save tracepoint definitions, full path"
+with_test_prefix "relative" {
+    do_save_load_test "savetrace-relative.tr"
+}

-gdb_delete_tracepoints
-gdb_test_no_output "set default-collect" "10.3: clear default-collect"
-gdb_test "info tracepoints" "No tracepoints." "10.3: delete tracepoints"
-gdb_test "source $trace_file_name" \
-	"Tracepoint \[0-9\]+ at .*" \
-	"10.4: read saved tracepoints, full path"
-gdb_verify_tracepoints "10.3: verify recovered tracepoints, full path"
-remote_file host delete $trace_file_name
-
-# 10.5 invalid filename
+with_test_prefix "absolute" {
+    do_save_load_test [standard_output_file "savetrace-absolute.tr"]
+}
+
+#      invalid filename
 #      [deferred -- not sure what a good invalid filename would be]

-# 10.6 save-trace (file already exists)
+#      save-trace (file already exists)
 #      [expect it to clobber the old one]

-# 10.7 help save-tracepoints
+# help save tracepoints

-gdb_test "help save-tracepoints" \
+gdb_test "help save tracepoints" \
 	"Save current tracepoint definitions as a script.*" \
-	"10.7: help save-tracepoints"
+	"verify help save tracepoints"
-- 
2.5.1