Re: [PATCH v3 5/5] check: add -L <n> parameter to rerun failed tests

[Zorro Lang <zlang@redhat.com>]

On Thu, Jul 07, 2022 at 08:09:36PM +0200, David Disseldorp wrote:
> On Wed, 6 Jul 2022 23:54:52 +0200, David Disseldorp wrote:
> 
> > Thanks for the follow-up feedback, Darrick...
> > 
> > On Wed, 6 Jul 2022 12:00:07 -0700, Darrick J. Wong wrote:
> ...
> > > >  
> > > > +# retain files which would be overwritten in subsequent reruns of the same test
> > > > +_stash_fail_loop_files() {
> > > > +	local test_seq="$1"
> > > > +	local suffix="$2"
> > > > +
> > > > +	for i in "${REPORT_DIR}/${test_seq}.full" \
> > > > +		 "${REPORT_DIR}/${test_seq}.dmesg" \
> > > > +		 "${REPORT_DIR}/${test_seq}.out.bad"; do
> > > > +		[ -f "$i" ] && cp "$i" "${i}${suffix}"    
> > > 
> > > I wonder, is there any particular reason to copy the output file and let
> > > it get overwritten instead of simply mv'ing it?  
> > 
> > The copy is left over from an earlier version I had where xunit report
> > generation was done after the copy. Looking closer:
> > - .full is removed in _begin_fstest()
> > - _check_dmesg() overwrites .dmesg and retains on failure or KEEP_DMESG
> > - out.bad is removed in the main check loop prior to seq invocation
> > - .notrun, .core and .hints are also removed in the check loop at
> >   various places before seq (.hints again in _begin_fstest())
> > 
> > One concern I have in changing this to a move is that external scripts
> > may check for presence / parse these files after check invocation. I'd
> > considered moving and then copying / symlinking back the .rerun0 files
> > on rerun-on-failure loop completion but that's also pretty ugly. IMO
> > leaving this as a copy, with the non-suffix file state left to reflect
> > the results of the last rerun-on-failure loop, would make the most
> > sense for now.
> 
> As a follow up here, I plan on squashing in the following change to
> cover the extra notrun, core and hints files, and also avoid stale
> .rerun# state:

OK, will wait your v4 of this patch, hope to catch the release of this week.

Thanks,
Zorro

> 
> --- a/check
> +++ b/check
> @@ -560,13 +560,14 @@ _expunge_test()
>  
>  # retain files which would be overwritten in subsequent reruns of the same test
>  _stash_fail_loop_files() {
> -       local test_seq="$1"
> -       local suffix="$2"
> +       local seq_prefix="${REPORT_DIR}/${1}"
> +       local cp_suffix="$2"
>  
> -       for i in "${REPORT_DIR}/${test_seq}.full" \
> -                "${REPORT_DIR}/${test_seq}.dmesg" \
> -                "${REPORT_DIR}/${test_seq}.out.bad"; do
> -               [ -f "$i" ] && cp "$i" "${i}${suffix}"
> +       for i in ".full" ".dmesg" ".out.bad" ".notrun" ".core" ".hints"; do
> +               rm -f "${seq_prefix}${i}${cp_suffix}"
> +               if [ -f "${seq_prefix}${i}" ]; then
> +                       cp "${seq_prefix}${i}" "${seq_prefix}${i}${cp_suffix}"
> +               fi
>         done
>  }
> 
> Cheers, David
>