Re: [PATCH v2 2/2] testsuite: Use `pktest` to test pickles and std

[Jose E. Marchesi]

Hi Mohammad!

> +# Simplified `verified_host_execute`
> +# The output pattern is relaxed to accept anything on each line.
> +# Only supports lines with {,X}PASSED, {,X}FAILED, UNTESTED, UNRESOLVED.
> +
> +proc simple_verified_host_execute {args} {
> +    set timeoutmsg "Timed out: Never got started, "
> +    set timeout 100
> +    set file all
> +    set timetol 0
> +    set arguments ""
> +
> +    if { [llength $args] == 0} {
> +        set executable $args
> +    } else {
> +        set executable [string trimleft [lindex [split $args " "] 0] "\{"]
> +        set params [string trimleft [lindex [split $args " "] 1] "\{"]
> +        set params [string trimright $params "\}"]
> +    }
> +
> +    verbose "The executable is $executable" 2
> +    if {![file exists ${executable}]} {
> +        perror "The executable, \"$executable\" is missing" 0
> +        return "No source file found"
> +    }
> +
> +    # spawn the executable and look for the DejaGnu output messages from the
> +    # test case.
> +    # spawn -noecho -open [open "|./${executable}" "r"]
> +    spawn -noecho "./${executable}" ${params}
> +    set prefix "\[^\r\n\]*"
> +    set txt "\[^\r\n\]*\r\n"
> +    expect {
> +        -re "^$prefix\tPASSED:${txt}" {
> +            set output [string range $expect_out(0,string) 8 end]
> +            pass "$output"
> +            set timetol 0
> +            exp_continue
> +        }
> +        -re "^$prefix\tXPASSED:${txt}" {
> +            set output [string range $expect_out(0,string) 8 end]
> +            xpass "$output"
> +            set timetol 0
> +            exp_continue
> +        }
> +        -re "^$prefix\tFAILED:${txt}" {
> +            set output [string range $expect_out(0,string) 8 end]
> +            fail "$output"
> +            set timetol 0
> +            exp_continue
> +        }
> +        -re "^$prefix\tXFAILED:${txt}" {
> +            set output [string range $expect_out(0,string) 8 end]
> +            xfail "$output"
> +            set timetol 0
> +            exp_continue
> +        }
> +        -re "^$prefix\tUNTESTED:${txt}" {
> +            set output [string range $expect_out(0,string) 8 end]
> +            untested "$output"
> +            set timetol 0
> +            exp_continue
> +        }
> +        -re "^$prefix\tUNRESOLVED:${txt}" {
> +            set output [string range $expect_out(0,string) 8 end]
> +            unresolved "$output"
> +            set timetol 0
> +            exp_continue
> +        }
> +        -re "^Totals" {
> +            verbose "All done" 2
> +        }
> +        eof {
> +            #       unresolved "${executable} died prematurely"
> +            #       catch close
> +            #       return "${executable} died prematurely"
> +        }
> +        timeout {
> +            warning "Timed out executing test case"
> +            if { $timetol <= 2 } {
> +                incr timetol
> +                exp_continue
> +            } else {
> +                catch close
> +                return "Timed out executing test case"
> +            }
> +        }
> +        -re "^$prefix\r\n" {
> +            exp_continue
> +        }
> +    }
> +
> +    catch wait wres
> +    verify_exit_status $executable $wres
> +
> +    # force a close of the executable to be safe.
> +    catch close
> +    return ""
> +}

Can't you parametrize the existing verified_host_execute instead?

> +++ b/testsuite/poke.pickles/Makefile.am
> @@ -0,0 +1,28 @@
> +# Copyright (C) 2020 The poke authors
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3, or (at your option)
> +# any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +AUTOMAKE_OPTIONS = subdir-objects
> +
> +CLEANFILES = run-pktests-pickles
> +check_PROGRAMS = run-pktests-pickles
> +
> +do_subst = \
> +  sed -e 's,[@]abs_top_builddir[@],$(abs_top_builddir),g' \
> +      -e 's,[@]abs_top_srcdir[@],$(abs_top_srcdir),g'     \
> +      -e 's,[@]test_component[@],pickles,g'
> +
> +run-pktests-pickles: ../run-pktests.in Makefile
> +     $(do_subst) < $(srcdir)/../run-pktests.in > run-pktests-pickles
> +     chmod +x run-pktests-pickles

You have rules like that in every subdirectory containing pktest tests,
processing the run-pktests.in below...

> diff --git a/testsuite/run-pktests.in b/testsuite/run-pktests.in
> new file mode 100644
> index 00000000..b6709073
> --- /dev/null
> +++ b/testsuite/run-pktests.in
> @@ -0,0 +1,37 @@
> +#!/bin/sh
> +
> +# Executes PkTests and translate TAP output to something understandable
> +# by DejaGNU.
> +
> +# Copyright (C) 2020 The poke authors
> +#
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +s="@abs_top_srcdir@"
> +b="@abs_top_builddir@"
> +c=@test_component@
> +
> +t() {
> +  "$b"/run poke -q -L "$1" \
> +  | awk '
> +    /^ok/     && / # SKIP/ { gsub(/^ok/,     "\tUNTESTED: "); print }
> +    /^ok/     && / # TODO/ { gsub(/^ok/,     "\tXPASSED: ");  print }
> +    /^not ok/ && / # TODO/ { gsub(/^not ok/, "\tXFAILED: ");  print }
> +    /^ok/                  { gsub(/^ok/,     "\tPASSED: ");   print }
> +    /^not ok/              { gsub(/^not ok/, "\tFAILED: ");   print }
> +  '
> +}
> +
> +find "$s"/testsuite/poke.$c/ -name '*-test.pk' \
> +| while read f; do t "$f"; done
 
... substituting the @...@ marks.  But I wonder, what about:

1) Rename run-pktests.in into run-pktests and put it in testsuite/lib.

2) You don't really need abs_top_builddir, because the runtest already
   has the right environment to run the uninstalled poke.  So you don't
   need to use the `run' script.  Just pass the Tcl variable $POKE to
   run-pktest and use it.

3) You don't need abs_top_srcdir.  The runtest script runs with cwd
   testsuite/, so in run-pktests you can simply do: find poke.$c/.


4) Make run-pktests get the `poke' and `test_component' as arguments:

   poke = $1
   component = $2
   
   ...
   t() {
     "$poke" -q -L ...

This way, you don't need the rewrites, and you could invoke the script
directly from the .exp files:

 load_lib "poke.exp"
 if { [simple_verified_host_execute "lib/run-pktests" $POKE pickles] ne "" } {
      fail "testing pickles failed"
  }

As an alternative to the above, have you considered to write run-pktests
as a Tcl procedure instead?  Then you could use it directly in:

 load_lib "poke.exp"
 if { [run_pktest pickles] ne "" } {
      fail "testing pickles failed"
  }