diff mbox series

gdb/testsuite: add untested message to skip_debuginfod_tests

Message ID OS3P286MB21524BD0F485B6991D4B87E4F01C9@OS3P286MB2152.JPNP286.PROD.OUTLOOK.COM
State New
Headers
Series gdb/testsuite: add untested message to skip_debuginfod_tests |

Commit Message

Enze Li Dec. 9, 2022, 11:56 a.m. UTC 
  When running the fetch_src_and_symbols.exp, I see the following output
which looks weird.  I even don't know whether the execution was successful.
======
Running /home/lee/dev/binutils-gdb/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp ...
testcase /home/lee/dev/binutils-gdb/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
completed in 0 seconds

                === gdb Summary ===

======

It is the fact that there's not enough feedback provided during the
skip_debuginfod_tests procedure.  Fix this by adding some untested
messages to the skip_debuginfod_tests procedure to clarify the output.

With this patch applied, I get:
======
Running /home/lee/dev/binutils-gdb/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp ...
UNTESTED: gdb.debuginfod/fetch_src_and_symbols.exp: cannot find debuginfo
testcase /home/lee/dev/binutils-gdb/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
completed in 0 seconds

                === gdb Summary ===

 # of untested testcases         1
======

I also modernized the form of 'if' with 'if {} {}' in
skip_debuginfod_tests.

Tested on x86_64-linux.
---
 gdb/testsuite/lib/debuginfod-support.exp | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)


base-commit: cd3866b6d07b37258eb840443537baa163877e24

Comments

Guinevere Larsen Dec. 12, 2022, 12:44 p.m. UTC | #1 
On 09/12/2022 12:56, Enze Li via Gdb-patches wrote:
> When running the fetch_src_and_symbols.exp, I see the following output
> which looks weird.  I even don't know whether the execution was successful.
> ======
> Running /home/lee/dev/binutils-gdb/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp ...
> testcase /home/lee/dev/binutils-gdb/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> completed in 0 seconds
>
>                  === gdb Summary ===
>
> ======
>
> It is the fact that there's not enough feedback provided during the
> skip_debuginfod_tests procedure.  Fix this by adding some untested
> messages to the skip_debuginfod_tests procedure to clarify the output.
>
> With this patch applied, I get:
> ======
> Running /home/lee/dev/binutils-gdb/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp ...
> UNTESTED: gdb.debuginfod/fetch_src_and_symbols.exp: cannot find debuginfo
> testcase /home/lee/dev/binutils-gdb/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> completed in 0 seconds
>
>                  === gdb Summary ===
>
>   # of untested testcases         1
> ======
>
> I also modernized the form of 'if' with 'if {} {}' in
> skip_debuginfod_tests.

Good catch!

Reviewed-By: Bruno Larsen <blarsen@redhat.com>
Tom Tromey Dec. 13, 2022, 4:14 p.m. UTC | #2 
>>>>> "Enze" == Enze Li via Gdb-patches <gdb-patches@sourceware.org> writes:

Enze>  proc skip_debuginfod_tests {} {
Enze> -    if [is_remote host] {
Enze> +    if { [is_remote host] } {
Enze> +	untested "does not work on remote host"
Enze>  	return true
Enze>      }

Do any of the other skip_ procs call untested?
I suspect this should be done somewhere else, and/or it's a pervasive
problem when a prerequisite is not met.

Tom
Enze Li Dec. 14, 2022, 7:43 a.m. UTC | #3 
On Tue, Dec 13 2022 at 09:14:47 AM -0700, Tom Tromey wrote:

>>>>>> "Enze" == Enze Li via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Enze>  proc skip_debuginfod_tests {} {
> Enze> -    if [is_remote host] {
> Enze> +    if { [is_remote host] } {
> Enze> +	untested "does not work on remote host"
> Enze>  	return true
> Enze>      }

Thanks for your swift replay.  :)

>
> Do any of the other skip_ procs call untested?

Hmmm...Not many.  Actually, from the skip_debuginfod_tests, I see an
untested message "cannot find curl".  It provides detailed information
when running the testcase.  Then I thought other spots in the same
procedure may benifit from it.

lib/debuginfod-support.exp:proc skip_debuginfod_tests {} {
lib/debuginfod-support.exp-    if [is_remote host] {
lib/debuginfod-support.exp-	return true
lib/debuginfod-support.exp-    }
lib/debuginfod-support.exp-
lib/debuginfod-support.exp-    if { [which debuginfod] == 0 } {
lib/debuginfod-support.exp-	return true
lib/debuginfod-support.exp-    }
lib/debuginfod-support.exp-
lib/debuginfod-support.exp-    if { [which curl] == 0 } {
lib/debuginfod-support.exp-	untested "cannot find curl"
lib/debuginfod-support.exp-	return true
lib/debuginfod-support.exp-    }

Except this, three unsupported messages were found in skip_* procedure. 

> I suspect this should be done somewhere else, and/or it's a pervasive
> problem when a prerequisite is not met.
>
> Tom

If the way of my previous patch is unconventional, how about this?

=================================================================
--- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
+++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
@@ -20,7 +20,24 @@ standard_testfile main.c
 load_lib dwarf.exp
 load_lib debuginfod-support.exp
 
-if { [skip_debuginfod_tests] } { return -1 }
+set skip_ret [skip_debuginfod_tests]
+if { $skip_ret < 0 } {
+    switch $skip_ret {
+	-1 {
+	    untested "does not work on remote host"
+	}
+	-2 {
+	    untested "cannot find debuginfo"
+	}
+	-3 {
+	    untested "cannot find curl"
+	}
+	-4 {
+	    untested "GDB was not configured with debuginfod"
+	}
+    }
+    return -1
+}
 
 set sourcetmp [standard_output_file tmp-${srcfile}]
 set outputdir [standard_output_file {}]

--- a/gdb/testsuite/lib/debuginfod-support.exp
+++ b/gdb/testsuite/lib/debuginfod-support.exp
@@ -18,17 +18,16 @@
 # Return true if the debuginfod tests should be skipped, otherwise, return
 # false.
 proc skip_debuginfod_tests {} {
-    if [is_remote host] {
-	return true
+    if { [is_remote host] } {
+	return -1
     }
 
     if { [which debuginfod] == 0 } {
-	return true
+	return -2
     }
 
     if { [which curl] == 0 } {
-	untested "cannot find curl"
-	return true
+	return -3
     }
 
     # Skip testing if gdb was not configured with debuginfod.
@@ -39,10 +38,10 @@ proc skip_debuginfod_tests {} {
     if { [string first "with-debuginfod" \
 	      [eval exec $::GDB --quiet $::INTERNAL_GDBFLAGS \
 		   --configuration]] == -1 } {
-	return true
+	return -4
     }
 
-    return false
+    return 0
 }
 
 # Create two directories within the current output directory.  One directory
=================================================================

Best Regards,
Enze
Tom Tromey Dec. 14, 2022, 5:19 p.m. UTC | #4 
>>>>> "Enze" == Enze Li via Gdb-patches <gdb-patches@sourceware.org> writes:

Enze> Hmmm...Not many.  Actually, from the skip_debuginfod_tests, I see an
Enze> untested message "cannot find curl".  It provides detailed information
Enze> when running the testcase.  Then I thought other spots in the same
Enze> procedure may benifit from it.

Personally I think these predicates should just return the result.  If
a message would be helpful, they can use 'verbose'.

Enze> If the way of my previous patch is unconventional, how about this?

I think it's kind of overkill.  "verbose" in the proc would work just as
well, and I suppose my feeling (unsupported by fact really) is that
normally an "untested" is just ignored anyway, and if you want to find
out what's happening you would have to dig a bit anyway.

Also I question if "untested" is correct due to what the dejagnu manual
says:

UNTESTED
     A test was not run.  This is a placeholder used when there is no
     real test case yet.

I think "unsupported" is what we ought to be using.

If you want to see how I think it should generally work, look at my
branch "rewrite-require".  This changes "require" to be simpler and then
tests can express their static requirements very simply:

require dwarf2 !isnative mumble bleah

Then the main difficulty is converting all the code to use this.
I made a stab at it there.

Tom
diff mbox series

Patch

diff --git a/gdb/testsuite/lib/debuginfod-support.exp b/gdb/testsuite/lib/debuginfod-support.exp
index ceecf9071110..c1a2bb900dce 100644
--- a/gdb/testsuite/lib/debuginfod-support.exp
+++ b/gdb/testsuite/lib/debuginfod-support.exp
@@ -18,11 +18,13 @@ 
 # Return true if the debuginfod tests should be skipped, otherwise, return
 # false.
 proc skip_debuginfod_tests {} {
-    if [is_remote host] {
+    if { [is_remote host] } {
+	untested "does not work on remote host"
 	return true
     }
 
     if { [which debuginfod] == 0 } {
+	untested "cannot find debuginfo"
 	return true
     }
 
@@ -39,6 +41,7 @@  proc skip_debuginfod_tests {} {
     if { [string first "with-debuginfod" \
 	      [eval exec $::GDB --quiet $::INTERNAL_GDBFLAGS \
 		   --configuration]] == -1 } {
+	untested "GDB was not configured with debuginfod"
 	return true
     }