Some tests on Windows are flaky because certain DLLs are writing to stdout, giving incorrect test results. We can work around that by duplicating and redirecting stdout.
Created attachment 298200 [details] Patch
Comment on attachment 298200 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298200&action=review > Tools/ChangeLog:3 > + [Win] Some tests are flaky. Not a very good bug title! > Tools/DumpRenderTree/PixelDumpSupport.cpp:64 > +#if PLATFORM(WIN) > + fprintf(fileStdout, "\nActualHash: %s\n", actualHash); // FIXME: No need for the leading newline. > +#else > printf("\nActualHash: %s\n", actualHash); // FIXME: No need for the leading newline. > +#endif Can we find a way to do this without an #if everywhere? I suggest we always have a variable like fileStdout, but just initialize it to point to stdout on non-Windows platforms. Maybe give it a different name. > Tools/DumpRenderTree/PixelDumpSupport.cpp:129 > printf("Content-Type: %s\n", "image/png"); The use of %s here is peculiar and also sort of pointless. We should fix that. > Tools/DumpRenderTree/win/DumpRenderTreeWin.h:46 > +extern FILE* fileStdout; I think that “file stdout” is a strange name for this. Instead can we just name it something that means "write test output here" and doesn’t necessarily mention "stdout" at all? After all, the call sites don’t really care that it’s stdout, they just care that it’s the correct place to write test output.
(In reply to comment #2) > Comment on attachment 298200 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=298200&action=review > > > Tools/ChangeLog:3 > > + [Win] Some tests are flaky. > > Not a very good bug title! > > > Tools/DumpRenderTree/PixelDumpSupport.cpp:64 > > +#if PLATFORM(WIN) > > + fprintf(fileStdout, "\nActualHash: %s\n", actualHash); // FIXME: No need for the leading newline. > > +#else > > printf("\nActualHash: %s\n", actualHash); // FIXME: No need for the leading newline. > > +#endif > > Can we find a way to do this without an #if everywhere? I suggest we always > have a variable like fileStdout, but just initialize it to point to stdout > on non-Windows platforms. Maybe give it a different name. > > > Tools/DumpRenderTree/PixelDumpSupport.cpp:129 > > printf("Content-Type: %s\n", "image/png"); > > The use of %s here is peculiar and also sort of pointless. We should fix > that. > > > Tools/DumpRenderTree/win/DumpRenderTreeWin.h:46 > > +extern FILE* fileStdout; > > I think that “file stdout” is a strange name for this. Instead can we just > name it something that means "write test output here" and doesn’t > necessarily mention "stdout" at all? After all, the call sites don’t really > care that it’s stdout, they just care that it’s the correct place to write > test output. Thanks for reviewing! I will update the patch.
Created attachment 298338 [details] Patch
Comment on attachment 298338 [details] Patch No need to review again, just uploaded to check that the revised patch passes tests :)
Committed r210503: <http://trac.webkit.org/changeset/210503>
Comment on attachment 298338 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298338&action=review Per, question about this old bug. > Tools/DumpRenderTree/win/DumpRenderTree.cpp:1117 > + // Check that test has not already run What is the good to check duplicated execution? It happens always when --iterations is more than one. Also I cannot find similar check on other ports.
(In reply to Basuke Suzuki from comment #7) > Comment on attachment 298338 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=298338&action=review > > Per, question about this old bug. > > > Tools/DumpRenderTree/win/DumpRenderTree.cpp:1117 > > + // Check that test has not already run > > What is the good to check duplicated execution? It happens always when > --iterations is more than one. Also I cannot find similar check on other > ports. I think this was used to track down an issue where the same tests appeared to be run multiple times. This turned out to not be the case, so I think this code can be removed.
Thanks! https://bugs.webkit.org/show_bug.cgi?id=188662