From: "Rémi Bernon" <rbernon@codeweavers.com>
Subject: Re: RFC: Allow marking unreliable tests as flaky.
Message-Id: <1f0dcfb9-000f-d15b-319f-bb83b68c347b@codeweavers.com>
Date: Wed, 1 Jun 2022 15:52:10 +0200
In-Reply-To: <E1nwONA-00GIlD-Gq@amboise>
References: <E1nwONA-00GIlD-Gq@amboise>

On 6/1/22 15:26, Francois Gouget wrote:
> 
> Wine has many unreliable tests which makes it hard to know whether a
> patch causes new failures. So one proposal was to mark unreliable tests
> as flaky, and have a mode where flaky failures can ignored by setting
> $WINETEST_ALLOW_FLAKY.
> 
> So one would be able to apply a patch, run 'WINETEST_ALLOW_FLAKY=1 make
> test' and if that fails that means the patch introduces a new failure.
> 
> The way it works is that when a flaky test fails the message is 'Flaky
> failed...' instead of 'Test failed...'. I also added the count of flaky
> failures to the summary line. So the flaky failures are not totally
> swept under the rug. The main difference is that if
> $WINETEST_ALLOW_FLAKY is set, the exit code is 0 if the only failures
> are flaky ones.
> 
> 
> That still leaves some open questions though:
> 
> * Should the message be 'Test failed' instead of 'Flaky failed' when
>    $WINETEST_ALLOW_FLAKY is not set? I opted for the latter because it
>    adds information that the failed test has been marked as flaky.
> 


Imho the message should keep the 'Test failed' / 'Test succeeded' 
prefixes. For instance I think it would look more consistent with 
something like:

   Test succeeded inside todo block:
   Test succeeded inside flaky block:
   Test marked todo:
   Test marked flaky:
   Test failed:
   Test succeeded:

> 
> * On a related note the patch below has flaky_windows and flaky_wine 
>   macros. Is that too fine grained? Maybe we don't want to run the risk 
>   of marking a test as flaky_windows only to discover later that it can 
>   also fail in Wine? But conversely, wouldn't it be valuable to know 
>   that a test is only flaky in Wine and not in Windows?
> 
>   Note: flaky_windows is a bit long but I was worried that flaky_win 
>         would be too similar to flaky_wine.
> 
> * I decided having flaky_{windows,wine}_if() macros would be overkill.

I think flaky_if should be enough, and then there could be some shortcut 
global variables for `!strcmp(winetest_platform, "windows" / "wine")`, 
so it's not too long to write.


Cheers,
-- 
Rémi Bernon <rbernon@codeweavers.com>