Bug 166760 – [Win] Some tests are flaky because certain DLLs are writing to stdout.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Per Arne Vollan]

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.

[Per Arne Vollan]

Created attachment 298200 [details]
Patch

[Darin Adler]

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.

[Per Arne Vollan]

(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.

[Per Arne Vollan]

Created attachment 298338 [details]
Patch

[Per Arne Vollan]

Comment on attachment 298338 [details]
Patch

No need to review again, just uploaded to check that the revised patch passes tests :)

[Per Arne Vollan]

Committed r210503: <http://trac.webkit.org/changeset/210503>

[Basuke Suzuki]

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.

[Per Arne Vollan]

(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.

[Basuke Suzuki]

Thanks! https://bugs.webkit.org/show_bug.cgi?id=188662