Re: [PATCH v6 16/21] mingw: try to work around issues with the test cleanup

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 19, 2021, 14:20 UTC
Message-ID: <nycvar.QRO.7.76.6.2103191515430.57@tvgsbejvaqbjf.bet>
In-Reply-To: <87zgzbgp1i.fsf@evledraar.gmail.com>

Hi Ævar,

thanks for digging out this old thread, I really could have done never thinking about it again!

On Wed, 10 Mar 2021, Ævar Arnfjörð Bjarmason wrote:

Show 40 quoted lines

>
> On Tue, Jan 29 2019, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > It seems that every once in a while in the Git for Windows SDK, there
> > are some transient file locking issues preventing the test clean up to
> > delete the trash directory. Let's be gentle and try again five seconds
> > later, and only error out if it still fails the second time.
> >
> > This change helps Windows, and does not hurt any other platform
> > (normally, it is highly unlikely that said deletion fails, and if it
> > does, normally it will fail again even 5 seconds later).
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  t/test-lib.sh | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index f31a1c8f79..9c0ca5effb 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -1104,7 +1104,11 @@ test_done () {
> >  			error "Tests passed but trash directory already removed before test cleanup; aborting"
> >
> >  			cd "$TRASH_DIRECTORY/.." &&
> > -			rm -fr "$TRASH_DIRECTORY" ||
> > +			rm -fr "$TRASH_DIRECTORY" || {
> > +				# try again in a bit
> > +				sleep 5;
> > +				rm -fr "$TRASH_DIRECTORY"
> > +			} ||
> >  			error "Tests passed but test cleanup failed; aborting"
> >  		fi
> >  		test_at_end_hook_
>
> I saw this sleep while reading some test-lib.sh code, doesn't this break
> df4c0d1a79 (test-lib: abort when can't remove trash directory,
> 2017-04-20) for non-Windows platforms?

It does not really break it, it just delays the inevitable failure.

> Your CL for v3 suggests this was only encountered in Azure VMs:
> https://lore.kernel.org/git/pull.31.v3.git.gitgitgadget@gmail.com/

If by "CL" you refer to the cover letter, then I might have made it sound as if it was only encountered in the Azure Pipelines agents. I vaguely seem to remember seeing something like this quite often on my personal machine, too, though. Most likely Microsoft Defender going a little wild.

> Aside from this obscure issue, wouldn't it make more sense to have some
> optional "I'm under CI" flag to skip the teardown one test at a time as
> we're probably about to shut off the transitory VM soon?

No, I'm not under CI, and I did encounter these issues. And they abruptly stopped with the patch you apparently still want to discuss ;-)

> I skip some tests, but the test suite creates ~950MB of trash for
> me. Maybe cheaper for some to just keep that around and have it all
> removed at the end.

I don't understand this statement. Or was there a question in it that you'd like me to answer?

Ciao, Johannes