Bug 503969

Testing upload:

$ cd <...>/auxprogs/auxchecks/ltp-full-20250130
$ t-upload-git-push ssh://builder.sourceware.org/git/bunsendb.git mcermak/PR503969-$(date +%s) $(find tests -type f )

Results: https://builder.sourceware.org/testrun/8079d12a755146a6487e85260880de4c20ecc72e

This is pretty hard to review without knowing what is intended.

As far as I can see there are a couple changes:
- ORIG_PWD isn't used anymore and is replace by SCRIPT_SRC=$(dirname $0)
  - This makes it possible to invoke the script from anywhere
- LOGDIR is redefined to no longer use the valgrind-ltp-logs but a more generic tests subdir
  - The buildbot relies on the old name and so should be adjusted to deal with "tests"
- The summary.log isn't generated anymore
- myLog isn't used anymore and is replaced by echo -e which means
  - That backslash escapes are now handled (no more literal \n in the output)
  - The individual test summary files aren't generated anymore
- Inside the LOGDIR a couple of files are generated:
  - test-suite.log which is kept empty? Was this intended to be the new summary.log?
  - Various .bunsen* files. Are these necessary when the buildbot does the bunsen uploading?
- Two automake style testlog files are generated
  - the $exe.log which contains something on FAIL, but is empty on PASS (is that intended?)
  - a $exe.trs file that contains the PASS or FAIL status.

Please review my understanding and update whether this is expected behaviour.

(In reply to mcermak from comment #1)
> Testing upload:
> 
> $ cd <...>/auxprogs/auxchecks/ltp-full-20250130
> $ t-upload-git-push ssh://builder.sourceware.org/git/bunsendb.git
> mcermak/PR503969-$(date +%s) $(find tests -type f )
> 
> Results:
> https://builder.sourceware.org/testrun/
> 8079d12a755146a6487e85260880de4c20ecc72e

Looks nice.

How does this compare to how the buildbot does the upload?
https://sourceware.org/cgit/builder/tree/builder/master.cfg#n5015
https://sourceware.org/cgit/builder/tree/builder/master.cfg#n1611

There is one more confusion about how bunsen/fake automake style logs are intended to work.
Where does the test-suite.log need to go?
For make regtest it is generated in a couple of places and then there is one "top-level" test-suite-overall.log

Hello Mark,

(In reply to Mark Wielaard from comment #2)
> This is pretty hard to review without knowing what is intended.
> 
> As far as I can see there are a couple changes:
> - ORIG_PWD isn't used anymore and is replace by SCRIPT_SRC=$(dirname $0)
>   - This makes it possible to invoke the script from anywhere

My original goal was to allow for a build directory separate from the source tree.  To be clear I mean doing something like ../valgrind/configure --prefix ....   This appears to partly work, but specifically the vg-in-place script still ends up being created within the source tree.  So, I gave up.  This relict change however looks good to me although it's not strictly needed for our use-case. 

> - LOGDIR is redefined to no longer use the valgrind-ltp-logs but a more
> generic tests subdir
>   - The buildbot relies on the old name and so should be adjusted to deal
> with "tests"

My intent in this case was to align the structure of the logs with the one coming from my RHEL valgrind-ltp tester here:
https://builder.sourceware.org/testruns/?op_mode=search_exact&has_keyvalue_k=testrun.gitdescribe&has_keyvalue_op=glob&has_keyvalue_v=*ltp*

> - The summary.log isn't generated anymore

Yes, that's intentional.  This ltp tester isn't actually automake based.  But the automake log format is the one I want to use for bunsen upload.  So this patch mimics the automake log format.  That said, for every testcase two log files are synthesized: a .log file containing the actual test log details, and a .trs file that encodes the test result (that's echo ":test-result: $rv" > $LOGDIR/$exe.trs).  There is one more thing the bunsen parser needs to successfully recognize our set of logs as automake based: the test-suite.log file.  No matter it is empty.  But it's there and with that bunsen can parse our logs as shown  above in Comment #1.  This hopefully explains most of the following notes too ...

> - myLog isn't used anymore and is replaced by echo -e which means
>   - That backslash escapes are now handled (no more literal \n in the output)
>   - The individual test summary files aren't generated anymore
> - Inside the LOGDIR a couple of files are generated:
>   - test-suite.log which is kept empty? Was this intended to be the new
> summary.log?
>   - Various .bunsen* files. Are these necessary when the buildbot does the
> bunsen uploading?

... except for this ^^.  These .bunsen files are there to provide more test run context to bunsen.  These need to be generated on the test system.  But it doesn't necessarily need to be part of valgrind source.  Another option is to make it part of the buildbot scripts responsible for executing the testsuite and uploading the test results to bunsen.  Moving this part out of this patch towards the build bot scripts is possible.

> - Two automake style testlog files are generated
>   - the $exe.log which contains something on FAIL, but is empty on PASS (is
> that intended?)
>   - a $exe.trs file that contains the PASS or FAIL status.
> 
> Please review my understanding and update whether this is expected behaviour.

Thank you for the review.  Please let me know if my comments don't answer your questions.  

Do you prefer to move generation of these .bunsen* files to the buildbot scripts?

Hi Martin,

(In reply to mcermak from comment #5)
> (In reply to Mark Wielaard from comment #2)
> > - ORIG_PWD isn't used anymore and is replace by SCRIPT_SRC=$(dirname $0)
> >   - This makes it possible to invoke the script from anywhere
> 
> My original goal was to allow for a build directory separate from the source
> tree.  To be clear I mean doing something like ../valgrind/configure
> --prefix ....   This appears to partly work, but specifically the
> vg-in-place script still ends up being created within the source tree.  So,
> I gave up.  This relict change however looks good to me although it's not
> strictly needed for our use-case. 

Yeah, the change is good. Lets keep it.
Sadly vg-in-place and make regtest don't work when srcdir != builddir (although make, make check, make dist[check] do).
 
> > - LOGDIR is redefined to no longer use the valgrind-ltp-logs but a more
> > generic tests subdir
> >   - The buildbot relies on the old name and so should be adjusted to deal
> > with "tests"
> 
> My intent in this case was to align the structure of the logs with the one
> coming from my RHEL valgrind-ltp tester here:
> https://builder.sourceware.org/testruns/
> ?op_mode=search_exact&has_keyvalue_k=testrun.
> gitdescribe&has_keyvalue_op=glob&has_keyvalue_v=*ltp*

Aha, ok, so those have results in a top-level 'tests' dir.
But I think it makes sense to upload the results of both regtests and ltpchecks together.
The buildbot script moves and renames the dir anyway.
We could move/rename it to ltp/tests ?

> > - The summary.log isn't generated anymore
> 
> Yes, that's intentional.  This ltp tester isn't actually automake based. 
> But the automake log format is the one I want to use for bunsen upload.  So
> this patch mimics the automake log format.  That said, for every testcase
> two log files are synthesized: a .log file containing the actual test log
> details, and a .trs file that encodes the test result (that's echo
> ":test-result: $rv" > $LOGDIR/$exe.trs).  There is one more thing the bunsen
> parser needs to successfully recognize our set of logs as automake based:
> the test-suite.log file.  No matter it is empty.  But it's there and with
> that bunsen can parse our logs as shown  above in Comment #1.  This
> hopefully explains most of the following notes too ...

Yes it does. Could we put something inside the test-suite.log file?
So that people who find it know why it is there and where to find the actual logs?
Maybe simply something like:
echo "See *.log files for details on each test in this directory." > test-suite.log

One thing that is different from how the regtests do this is that they put a test-suite.log file in each directory where there are test .log/.trs files? Why is that necessary there and not here?

> > - myLog isn't used anymore and is replaced by echo -e which means
> >   - That backslash escapes are now handled (no more literal \n in the output)
> >   - The individual test summary files aren't generated anymore
> > - Inside the LOGDIR a couple of files are generated:
> >   - test-suite.log which is kept empty? Was this intended to be the new
> > summary.log?
> >   - Various .bunsen* files. Are these necessary when the buildbot does the
> > bunsen uploading?
> 
> ... except for this ^^.  These .bunsen files are there to provide more test
> run context to bunsen.  These need to be generated on the test system.  But
> it doesn't necessarily need to be part of valgrind source.  Another option
> is to make it part of the buildbot scripts responsible for executing the
> testsuite and uploading the test results to bunsen.  Moving this part out of
> this patch towards the build bot scripts is possible.

Lets do that. If we upload everything at the same time, bunsen would also get the config.log file to determine the configuration.

> > - Two automake style testlog files are generated
> >   - the $exe.log which contains something on FAIL, but is empty on PASS (is
> > that intended?)
> >   - a $exe.trs file that contains the PASS or FAIL status.
> > 
> > Please review my understanding and update whether this is expected behaviour.
> 
> Thank you for the review.  Please let me know if my comments don't answer
> your questions. 

The only unanswered question is whether the $exe.log file should be empty when the test PASSes.
Don't we want some logs even if something passes?
 
> Do you prefer to move generation of these .bunsen* files to the buildbot
> scripts?

Yes.

Created attachment 181372 [details]
proposed patch

(In reply to Mark Wielaard from comment #6)
> > > - LOGDIR is redefined to no longer use the valgrind-ltp-logs but a more
> > > generic tests subdir
> > >   - The buildbot relies on the old name and so should be adjusted to deal
> > > with "tests"
> > 
> > My intent in this case was to align the structure of the logs with the one
> > coming from my RHEL valgrind-ltp tester here:
> > https://builder.sourceware.org/testruns/
> > ?op_mode=search_exact&has_keyvalue_k=testrun.
> > gitdescribe&has_keyvalue_op=glob&has_keyvalue_v=*ltp*
> 
> Aha, ok, so those have results in a top-level 'tests' dir.
> But I think it makes sense to upload the results of both regtests and
> ltpchecks together.
> The buildbot script moves and renames the dir anyway.
> We could move/rename it to ltp/tests ?

Absolutely.  

Frank's suggestion:
> ...
> < fche> yes, that works.  if OTOH the ltp tests are already structured into subdirs that benefit from 
>              preserving (for collision avoidance or whateve reasons), then N subdirs are fine too
> ...

Taking into account Frank's suggestion to preserve the LTP
testcase directory structure, I've renamed it to ltp/testcases.  With the
attached patch the structure looks like this:

...
TESTING FINISHED, logs in /home/mcermak/WORK/valgrind/valgrind/auxprogs/auxchecks/ltp-full-20250130/ltp
make[1]: Leaving directory '/home/mcermak/WORK/valgrind/valgrind/auxprogs'
41$ find . -type f -name '*.log' -o -name  '*.trs' -o -name 'test-suite.log' | head
./testcases/kernel/syscalls/sched_get_priority_max/sched_get_priority_max01/sched_get_priority_max01.trs
./testcases/kernel/syscalls/sched_get_priority_max/sched_get_priority_max01/test-suite.log
./testcases/kernel/syscalls/sched_get_priority_max/sched_get_priority_max01/sched_get_priority_max01.log
./testcases/kernel/syscalls/sched_get_priority_max/sched_get_priority_max02/sched_get_priority_max02.trs
./testcases/kernel/syscalls/sched_get_priority_max/sched_get_priority_max02/test-suite.log
./testcases/kernel/syscalls/sched_get_priority_max/sched_get_priority_max02/sched_get_priority_max02.log
./testcases/kernel/syscalls/sigpending/sigpending02/sigpending02.log
./testcases/kernel/syscalls/sigpending/sigpending02/sigpending02.trs
./testcases/kernel/syscalls/sigpending/sigpending02/test-suite.log
./testcases/kernel/syscalls/renameat2/renameat201/renameat201.log
41$

> Could we put something inside the test-suite.log file?
> So that people who find it know why it is there and where to find the actual
> logs?
> Maybe simply something like:
> echo "See *.log files for details on each test in this directory." >
> test-suite.log

As I was attempting to preserve the LTP directory strucure, I've found that it can be done, BUT
the test-suite.log needs to be present in every subdirectory containing .trs and .log files.  Instead
of putting the above message to it, I've put the test timing information to it.  I thought this 
might be useful at some point.  We can also add the info message you've suggested if you like it
better.

> One thing that is different from how the regtests do this is that they put a
> test-suite.log file in each directory where there are test .log/.trs files?
> Why is that necessary there and not here?

Yeah, that appears to be needed so that bunsen can recognize and parse the logs. 

> The only unanswered question is whether the $exe.log file should be empty
> when the test PASSes.
> Don't we want some logs even if something passes?
>  
> > Do you prefer to move generation of these .bunsen* files to the buildbot
> > scripts?

Done! :)

I've generated new test data using the attached patch and uploaded those to bunsen:
https://builder.sourceware.org/testrun/3036eba2de1cc44a1942f9681720ea234da9029e

Please review the attached patch and let me know your thoughts.

(In reply to mcermak from comment #7)
> Please review the attached patch and let me know your thoughts.

This looks really good. Thanks. I renamed the results dir in the buildbot to ltp, added bug 503969 to NEWS and pushed as:

commit 5894abc5fa9de30c6b4dde453bff3ac1034aa330
Author: Martin Cermak <mcermak@redhat.com>
Date:   Fri May 16 11:46:06 2025 +0200

    PR503969: Make test results of make ltpchecks compatible with bunsen
    
    Synthesize automake-like testcase log format for bunsen compatibility.
    
    Each testcase now produces a triplet of <testcase>.{log,trs}, and a
    test-suite.log file.  This way test results can be uploaded to bunsen
    using the t-upload-git-push command.
    
    The <testcase>.log file contains the actual testcase log. The .trs
    file contains automake-encoded test result.  The test-suite.log
    file contains testcase timing information.
    
    The log directory tree respect the one of the LTP testcases to avoid
    naming collisions.
    
    Here is how the test result was uploaded to Bunsen:
    
    > TESTING FINISHED, logs in /home/mcermak/WORK/valgrind/valgrind/auxprogs/auxchecks/ltp-full-20250130/ltp
    > make[1]: Leaving directory '/home/mcermak/WORK/valgrind/valgrind/auxprogs'
    >
    > real  8m27.467s
    > user  44m46.416s
    > sys   6m46.405s
    > 41$ cd /home/mcermak/WORK/valgrind/valgrind/auxprogs/auxchecks/ltp-full-20250130/ltp
    > 41$ t-upload-git-push ssh://builder.sourceware.org/git/bunsendb.git mcermak/PR503969-$(date +%s) \
    >  $(find . -type f -name '*.log' -o -name  '*.trs' -o -name 'test-suite.log')
    > 3036eba2de1cc44a1942f9681720ea234da9029e refs/tags/mcermak/PR503969-1747389329
    > 41$
    
    Here's the resulting bunsen upload:
    https://builder.sourceware.org/testrun/3036eba2de1cc44a1942f9681720ea234da9029e

GIven that the naming structure of ltp tests is carefully designed to be non-conflicting:

    ltp/testcases/kernel/syscalls/SYSCALL/SYSCALL###/SYSCALL###.{log.trs}

it seems to me we don't really need so many subdirectories.  They could all go into one:

   ltp/testcases/kernel/syscalls/SYSCALL###.{log.trs}
   ltp/testcases/kernel/syscalls/test-suite.log

or if someone really feels that's too much, then one subdir per syscall (not per syscall TEST):

   ltp/testcases/kernel/syscalls/SYSCALL/SYSCALL###.{log.trs}
   ltp/testcases/kernel/syscalls/SYSCALL/test-suite.log

Created attachment 181397 [details]
proposed patch

Hi Frank and Mark,

thank you for the review and buildbot integration.  Nice to see it working!
I've attached a patch flattening the directory structure.  Testing upload:

...
TESTING FINISHED, logs in /home/mcermak/WORK/valgrind/valgrind/auxprogs/auxchecks/ltp-full-20250130/ltp/tests
make[1]: Leaving directory '/home/mcermak/WORK/valgrind/valgrind/auxprogs'
41$ cd /home/mcermak/WORK/valgrind/valgrind/auxprogs/auxchecks/ltp-full-20250130/
41$ t-upload-git-push ssh://builder.sourceware.org/git/bunsendb.git mcermak/PR503969-$(date +%s) $(find ltp -type f )
5b8f868b3e3c84801814dcd4ea963690f94fd2d1 refs/tags/mcermak/PR503969-1747426239
41$

Bunsen URL: https://builder.sourceware.org/testrun/5b8f868b3e3c84801814dcd4ea963690f94fd2d1

Looks good, pushed as:

commit 2045aefbb0261bd5844b693e5d390affcffb8749
Author: Martin Cermak <mcermak@redhat.com>
Date:   Fri May 16 22:12:39 2025 +0200

    PR503969: make ltpchecks: flatten the log structure
    
    Flatten the directory structure of make ltpchecks logs per PR503969#c9.
    Individual syscall tests are numbered, so that no testcase naming
    conflicts should show up.  Demo upload:
    
    https://builder.sourceware.org/testrun/5b8f868b3e3c84801814dcd4ea963690f94fd2d1