Re: [Fuego] [PATCH 4/4] rt: search for the binary if the build phase is skipped

Hi Tim,

Thanks again for your review.
We will fix the patch and re-send.

See my comments inline:

> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: Saturday, August 21, 2021 5:16 AM
> To: sangorrin daniel(サンゴリン ダニエル □ＳＷＣ◯ＡＣＴ) <[email protected]>
> Cc: [email protected]; nguyen dat tho(ＴＳＤＶ Ｅｎｇ　１) <[email protected]>
> Subject: RE: [PATCH 4/4] rt: search for the binary if the build phase is skipped
> 
> > -----Original Message-----
> > From: Daniel Sangorrin <[email protected]>
> >
> > From: Nguyen Dat Tho <[email protected]>
> >
> > ftc has the ability to skip the build phase to use a previously built
> > binary or a binary installed on the board's file system (e.g. apt-get
> > install rt-tests).
> >
> > Signed-off-by: Nguyen Dat Tho <[email protected]>
> > Signed-off-by: Daniel Sangorrin <[email protected]>
> > ---
> >  tests/Benchmark.cyclictest/fuego_test.sh  | 7 ++++++-
> >  tests/Benchmark.hackbench/fuego_test.sh   | 7 ++++++-
> >  tests/Benchmark.migratetest/fuego_test.sh | 7 ++++++-
> >  tests/Benchmark.pmqtest/fuego_test.sh     | 8 +++++++-
> >  tests/Benchmark.ptsematest/fuego_test.sh  | 8 +++++++-
> > tests/Benchmark.signaltest/fuego_test.sh  | 7 ++++++-
> > tests/Benchmark.sigwaittest/fuego_test.sh | 7 ++++++-
> > tests/Benchmark.svsematest/fuego_test.sh  | 7 ++++++-
> >  tests/Functional.pi_tests/fuego_test.sh   | 7 ++++++-
> >  9 files changed, 56 insertions(+), 9 deletions(-)
> >
> > diff --git a/tests/Benchmark.cyclictest/fuego_test.sh
> > b/tests/Benchmark.cyclictest/fuego_test.sh
> > index 74d9d24..e7070dd 100755
> > --- a/tests/Benchmark.cyclictest/fuego_test.sh
> > +++ b/tests/Benchmark.cyclictest/fuego_test.sh
> > @@ -24,5 +24,10 @@ function test_deploy {  }
> >
> >  function test_run {
> > -    report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./cyclictest $BENCHMARK_CYCLICTEST_PARAMS"
> > +    if [ -f $BOARD_TESTDIR/fuego.$TESTDIR/cyclictest ]; then
> This test ([ -f) won't work on a remote board.  This is running on the host machine, which has a different filesystem than the device
> under test, unless you are running using the 'local' TRANSPORT.
> 
> There should be a cmd() in here somewhere to perform this test on the board's filesystem.

Yes, you are totally right. Sorry for not catching that problem during my review.
 
> I'm not sure I follow this.  What does the test_deploy() function look like?
> Under what circumstances would cyclictest not be present in the boards $BOARD_TESTDIR/fuego.$TESTDIR directory?

We want to run fuego natively because it is easier to run it on certain scenarios such as LAVA board farms and custom cloud images.
We also want to use packaged versions of the tests and do not store any test tarball in our repositories. So the OS image will already have the tests there and therefore we will skip the build/deploy phases.
 
> It seems like this code should be coupled with code that detects if the program is already present, and if so 1) avoids deploying it and

We are skipping the build and deploy phase on purpose from ftc.

> 2) then uses it for the test.
> 
> Something like this:
> test_deploy {
>   is_on_target_path cyclictest PROGRAM_CYCLICTEST
>   if [ -z "$PROGRAM_CYCLICTEST" ] ; then
>      put cyclictest $BOARD_TESTDIR/fuego.$TESTDIR
>      PROGRAM_CYCLICTEST=$BOARD_TESTDIR/fuego.$TESTDIR/cyclictest
>  fi
>   export PROGRAM_CYCLICTEST
> }
> 
> and then in test_run:
>   if [ -n "$PROGRAM_CYCLICTEST" ] ; then
>     report "$PROGRAM_CYCLICTEST $BENCHMARK_CYCLICTEST_PARAMS"
>   else
>     abort_job "cyclictest is not found on the target"
>   fi
> 
> Maybe I'm not understanding the use case here.  Is this for running a pre-existing program on the board, or a program that was already
> placed in the BOARD_TESTDIR directory (e.g. from a previous run of the test)?

A pre-existing program on the board (e.g. previously installed). 

> If we have this pattern a lot, then maybe it would make sense to make the optional deploy code  a core function, like:
>    put_if_program_not_present cyclictest

We are skipping the build/deploy phase using ftc. What do you think?

Thanks,
Daniel


> 
>  -- Tim
> 
> >  }
> > diff --git a/tests/Benchmark.hackbench/fuego_test.sh
> > b/tests/Benchmark.hackbench/fuego_test.sh
> > index cc05c74..da209ff 100755
> > --- a/tests/Benchmark.hackbench/fuego_test.sh
> > +++ b/tests/Benchmark.hackbench/fuego_test.sh
> > @@ -16,5 +16,10 @@ function test_deploy {  }
> >
> >  function test_run {
> > -    report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./hackbench $BENCHMARK_HACKBENCH_PARAMS"
> > +    if [ -f $BOARD_TESTDIR/fuego.$TESTDIR/hackbench ]; then
> > +        report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./hackbench $BENCHMARK_HACKBENCH_PARAMS"
> > +    else
> > +        assert_has_program hackbench
> > +        report "$PROGRAM_HACKBENCH $BENCHMARK_HACKBENCH_PARAMS"
> > +    fi
> >  }
> > diff --git a/tests/Benchmark.migratetest/fuego_test.sh
> > b/tests/Benchmark.migratetest/fuego_test.sh
> > index eeaa4f6..0b22818 100755
> > --- a/tests/Benchmark.migratetest/fuego_test.sh
> > +++ b/tests/Benchmark.migratetest/fuego_test.sh
> > @@ -23,5 +23,10 @@ function test_deploy {  }
> >
> >  function test_run {
> > -    report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./${TEST_COMMAND} $BENCHMARK_MIGRATETEST_PARAMS"
> > +    if [ -f $BOARD_TESTDIR/fuego.$TESTDIR/$TEST_COMMAND ]; then
> > +        report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./${TEST_COMMAND} $BENCHMARK_MIGRATETEST_PARAMS"
> > +    else
> > +        assert_has_program $TEST_COMMAND
> > +        report "$PROGRAM_RT_MIGRATE_TEST $BENCHMARK_MIGRATETEST_PARAMS"
> > +    fi
> >  }
> > diff --git a/tests/Benchmark.pmqtest/fuego_test.sh
> > b/tests/Benchmark.pmqtest/fuego_test.sh
> > index 6733e78..8d3f75a 100755
> > --- a/tests/Benchmark.pmqtest/fuego_test.sh
> > +++ b/tests/Benchmark.pmqtest/fuego_test.sh
> > @@ -27,5 +27,11 @@ function test_run {
> >      # The number for getting the lines depends on the cpu number of target machine.
> >      target_cpu_number=$(cmd "nproc")
> >      getting_line_number=$(( $target_cpu_number + $target_cpu_number ))
> > -    report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./pmqtest $BENCHMARK_PMQTEST_PARAMS | tail -$getting_line_number"
> > +
> > +    if [ -f $BOARD_TESTDIR/fuego.$TESTDIR/pmqtest ]; then
> > +        report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./pmqtest $BENCHMARK_PMQTEST_PARAMS | tail -$getting_line_number"
> > +    else
> > +        assert_has_program pmqtest
> > +        report "$PROGRAM_PMQTEST $BENCHMARK_PMQTEST_PARAMS | tail -$getting_line_number"
> > +    fi
> >  }
> > diff --git a/tests/Benchmark.ptsematest/fuego_test.sh
> > b/tests/Benchmark.ptsematest/fuego_test.sh
> > index a626d44..e534412 100755
> > --- a/tests/Benchmark.ptsematest/fuego_test.sh
> > +++ b/tests/Benchmark.ptsematest/fuego_test.sh
> > @@ -27,5 +27,11 @@ function test_run {
> >      # The number for getting the lines depends on the cpu number of target machine.
> >      target_cpu_number=$(cmd "cat /proc/cpuinfo | grep processor | wc -l")
> >      getting_line_number=$(( $target_cpu_number + $target_cpu_number ))
> > -    report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./ptsematest $BENCHMARK_PTSEMATEST_PARAMS | tail -$getting_line_number"
> > +
> > +    if [ -f $BOARD_TESTDIR/fuego.$TESTDIR/ptsematest ]; then
> > +        report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./ptsematest $BENCHMARK_PTSEMATEST_PARAMS | tail -
> $getting_line_number"
> > +    else
> > +        assert_has_program ptsematest
> > +        report "$PROGRAM_PTSEMATEST $BENCHMARK_PTSEMATEST_PARAMS | tail -$getting_line_number"
> > +    fi
> >  }
> > diff --git a/tests/Benchmark.signaltest/fuego_test.sh
> > b/tests/Benchmark.signaltest/fuego_test.sh
> > index 54e08b1..8811d17 100755
> > --- a/tests/Benchmark.signaltest/fuego_test.sh
> > +++ b/tests/Benchmark.signaltest/fuego_test.sh
> > @@ -22,5 +22,10 @@ function test_deploy {  }
> >
> >  function test_run {
> > -    report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./signaltest $BENCHMARK_SIGNALTEST_PARAMS"
> > +    if [ -f $BOARD_TESTDIR/fuego.$TESTDIR/signaltest ]; then
> > +        report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./signaltest $BENCHMARK_SIGNALTEST_PARAMS"
> > +    else
> > +        assert_has_program signaltest
> > +        report "$PROGRAM_SIGNALTEST $BENCHMARK_SIGNALTEST_PARAMS"
> > +    fi
> >  }
> > diff --git a/tests/Benchmark.sigwaittest/fuego_test.sh
> > b/tests/Benchmark.sigwaittest/fuego_test.sh
> > index e419f17..babcab6 100755
> > --- a/tests/Benchmark.sigwaittest/fuego_test.sh
> > +++ b/tests/Benchmark.sigwaittest/fuego_test.sh
> > @@ -27,5 +27,10 @@ function test_run {
> >      # The number for getting the lines depends on the cpu number of target machine.
> >      target_cpu_number=$(cmd "nproc")
> >      getting_line_number=$(( $target_cpu_number + $target_cpu_number ))
> > -    report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./sigwaittest $BENCHMARK_SIGWAITTEST_PARAMS | tail -$getting_line_number"
> > +    if [ -f $BOARD_TESTDIR/fuego.$TESTDIR/sigwaittest ]; then
> > +        report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./sigwaittest $BENCHMARK_SIGWAITTEST_PARAMS | tail -
> $getting_line_number"
> > +    else
> > +        assert_has_program sigwaittest
> > +        report "$PROGRAM_SIGWAITTEST $BENCHMARK_SIGWAITTEST_PARAMS | tail -$getting_line_number"
> > +    fi
> >  }
> > diff --git a/tests/Benchmark.svsematest/fuego_test.sh
> > b/tests/Benchmark.svsematest/fuego_test.sh
> > index 2f6e914..cb0184b 100755
> > --- a/tests/Benchmark.svsematest/fuego_test.sh
> > +++ b/tests/Benchmark.svsematest/fuego_test.sh
> > @@ -27,5 +27,10 @@ function test_run {
> >      # The number for getting the lines depends on the cpu number of target machine.
> >      target_cpu_number=$(cmd "cat /proc/cpuinfo | grep processor | wc -l")
> >      getting_line_number=$(( $target_cpu_number + $target_cpu_number ))
> > -    report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./svsematest $BENCHMARK_SVSEMATEST_PARAMS | tail -$getting_line_number"
> > +    if [ -f $BOARD_TESTDIR/fuego.$TESTDIR/svsematest ]; then
> > +        report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./svsematest $BENCHMARK_SVSEMATEST_PARAMS | tail -
> $getting_line_number"
> > +    else
> > +        assert_has_program svsematest
> > +        report "$PROGRAM_SVSEMATEST $BENCHMARK_SVSEMATEST_PARAMS | tail -$getting_line_number"
> > +    fi
> >  }
> > diff --git a/tests/Functional.pi_tests/fuego_test.sh
> > b/tests/Functional.pi_tests/fuego_test.sh
> > index bf94a63..590fbf3 100755
> > --- a/tests/Functional.pi_tests/fuego_test.sh
> > +++ b/tests/Functional.pi_tests/fuego_test.sh
> > @@ -22,7 +22,12 @@ function test_deploy {  }
> >
> >  function test_run {
> > -    report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./pi_stress $FUNCTIONAL_PI_TESTS_PARAMS"
> > +    if [ -f $BOARD_TESTDIR/fuego.$TESTDIR/pi_stress ]; then
> > +        report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./pi_stress $FUNCTIONAL_PI_TESTS_PARAMS"
> > +    else
> > +        assert_has_program pi_stress
> > +        report "$PROGRAM_PI_STRESS $FUNCTIONAL_PI_TESTS_PARAMS"
> > +    fi
> >  }
> >
> >  function test_processing {
> > --
> > 2.17.1
> >