Marking test cases as flaky?
Hi folks, I've been running "make test" a lot lately, and occasionally (it happened five times today), a test will fail out of the blue, in one out of twenty or a hundred runs. It would be bad form for buildbot to reject a patch because of a spurious test failure, so I've been making a list of tests that fail, adding to it each time I run into a test that fails occasionally, and skipping the tests on that list during "make test". This is all fine... but one of the tests that fails occasionally is ddraw/visual.ok (see http://bugs.winehq.org/show_bug.cgi?id=28301 ). It really doesn't feel right to skip that entirely just because a few particular pixel comparisons fail sometimes. Plan A would be to fix the tests to not be flaky, but that's hard, and the buildbot needs a quick solution. Plan B might be to conditionally compile the flaky tests away, e.g.: --- a/dlls/ddraw/tests/visual.c +++ b/dlls/ddraw/tests/visual.c @@ -1490,6 +1490,7 @@ static void D3D1_TextureMapBlendTest(void) ok(hr == D3D_OK, "IDirect3DDevice3_EndScene failed, hr = %08x\n", hr); } +#ifndef WINE_SKIP_FLAKY_TEST_SEE_BUG_28301 color = D3D1_getPixelColor(DirectDraw1, Surface1, 5, 5); red = (color & 0x00ff0000) >> 16; green = (color & 0x0000ff00) >> 8; @@ -1513,6 +1514,7 @@ static void D3D1_TextureMapBlendTest(void) green = (color & 0x0000ff00) >> 8; blue = (color & 0x000000ff); ok(red == 0 && green == 0 && blue == 0xff, "Got color %08x, expected 000000ff or near\n", color); +#endif Then my blacklist would mostly be a bunch of -D definitions in CFLAGS, one per flaky test. Or we could use a single define for all flaky tests. I may try that privately, to increase buildbot's test coverage. Would something like that be useful for other people, or should I just stick with private patches? - Dan
Am 07.09.2011 06:14, schrieb Dan Kegel:
Hi folks, I've been running "make test" a lot lately, and occasionally (it happened five times today), a test will fail out of the blue, in one out of twenty or a hundred runs. It would be bad form for buildbot to reject a patch because of a spurious test failure, so I've been making a list of tests that fail, adding to it each time I run into a test that fails occasionally, and skipping the tests on that list during "make test". This is all fine... but one of the tests that fails occasionally is ddraw/visual.ok (see http://bugs.winehq.org/show_bug.cgi?id=28301 ). It really doesn't feel right to skip that entirely just because a few particular pixel comparisons fail sometimes.
Plan A would be to fix the tests to not be flaky, but that's hard, and the buildbot needs a quick solution.
Plan B might be to conditionally compile the flaky tests away, e.g.:
Plan C could be to do what testbot does and remember possible fails, maybe including the out of the blue ones. -- Best Regards, André Hentschel
André wrote:
Plan C could be to do what testbot does and remember possible fails, maybe including the out of the blue ones.
Testbot still suffers from a lot of false positives, I think. Yes, a build bot can do all sorts of workarounds, but we shouldn't forget about normal developers. It would be great if the average developer got in the habit of running 'make test' frequently and could rely on it not failing randomly.
A problem we have is that we have lots and lots of tests in dlls/d3d*/visual.c and device.c that are unrelated to each other. d3d9/visual has about 100 separate tests that could be in different files and work just the same way. Now I don't suggest moving them into different files, that would make the directory look pretty ugly. But maybe we could introduce a function that gives all following ok() calls a different name and makes it easier for external apps to separate tests. E.g. wine_begin_tests(depth_clamp); ok(...); ok(...); ok(...); wine_begin_tests(stretchrect) ok(...); ok(...); ok(...); and so on.
On Sat, Sep 10, 2011 at 3:01 AM, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
A problem we have is that we have lots and lots of tests in dlls/d3d*/visual.c and device.c that are unrelated to each other. d3d9/visual has about 100 separate tests that could be in different files and work just the same way.
Now I don't suggest moving them into different files, that would make the directory look pretty ugly. But maybe we could introduce a function that gives all following ok() calls a different name and makes it easier for external apps to separate tests. E.g.
wine_begin_tests(depth_clamp); ok(...); ok(...); ok(...); wine_begin_tests(stretchrect) ok(...); ok(...); ok(...);
Maybe instead of #define ok_(file, line) (winetest_set_location(file, line), 0) ? (void)0 : winetest_ok #define ok ok_(__FILE__, __LINE__) we could have #define ok_(file, func, line) (winetest_set_location(file, func, line), 0) ? (void)0 : winetest_ok #define ok ok_(__FILE__, __FUNCTION__, __LINE__) That would get you most of what you propose without any changes to the tests, I think.
On Saturday 10 September 2011 19:43:08 Dan Kegel wrote:
#define ok_(file, func, line) (winetest_set_location(file, func, line), 0) ? (void)0 : winetest_ok #define ok ok_(__FILE__, __FUNCTION__, __LINE__)
That would get you most of what you propose without any changes to the tests, I think. I think that would work. Of course you have to change the tests that explicitly use ok_()
On Sat, Sep 10, 2011 at 10:55 AM, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
On Saturday 10 September 2011 19:43:08 Dan Kegel wrote:
#define ok_(file, func, line) (winetest_set_location(file, func, line), 0) ? (void)0 : winetest_ok #define ok ok_(__FILE__, __FUNCTION__, __LINE__)
That would get you most of what you propose without any changes to the tests, I think. I think that would work. Of course you have to change the tests that explicitly use ok_()
Might want to define the new form as ok2_ or something so we can defer changing the explicit uses of ok_().
On Saturday 10 September 2011 21:56:26 Dan Kegel wrote:
Might want to define the new form as ok2_ or something so we can defer changing the explicit uses of ok_(). I dislike the idea, it has the feeling of legacy cruft. Either way it is a fairly minor point - the main change that needs debating is whether we should print the function name or not.
Stefan Dösinger <stefandoesinger(a)gmx.at> writes:
On Saturday 10 September 2011 21:56:26 Dan Kegel wrote:
Might want to define the new form as ok2_ or something so we can defer changing the explicit uses of ok_(). I dislike the idea, it has the feeling of legacy cruft. Either way it is a fairly minor point - the main change that needs debating is whether we should print the function name or not.
We don't want that by default, especially since __FUNCTION__ is not portable. If you have cases where you get frequent failures that are hard to debug, you can always print extra info in the ok message. -- Alexandre Julliard julliard(a)winehq.org
participants (4)
-
Alexandre Julliard -
André Hentschel -
Dan Kegel -
Stefan Dösinger