Bug 166760 - [Win] Some tests are flaky because certain DLLs are writing to stdout.
Summary: [Win] Some tests are flaky because certain DLLs are writing to stdout.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-06 07:35 PST by Per Arne Vollan
Modified: 2018-08-16 11:11 PDT (History)
4 users (show)

See Also:


Attachments
Patch (55.30 KB, patch)
2017-01-06 07:46 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (55.40 KB, patch)
2017-01-09 02:56 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2017-01-06 07:35:31 PST
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.
Comment 1 Per Arne Vollan 2017-01-06 07:46:45 PST
Created attachment 298200 [details]
Patch
Comment 2 Darin Adler 2017-01-06 13:55:10 PST
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.
Comment 3 Per Arne Vollan 2017-01-09 02:37:02 PST
(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.
Comment 4 Per Arne Vollan 2017-01-09 02:56:07 PST
Created attachment 298338 [details]
Patch
Comment 5 Per Arne Vollan 2017-01-09 04:37:54 PST
Comment on attachment 298338 [details]
Patch

No need to review again, just uploaded to check that the revised patch passes tests :)
Comment 6 Per Arne Vollan 2017-01-09 05:00:34 PST
Committed r210503: <http://trac.webkit.org/changeset/210503>
Comment 7 Basuke Suzuki 2018-08-16 10:30:05 PDT
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.
Comment 8 Per Arne Vollan 2018-08-16 10:37:08 PDT
(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.
Comment 9 Basuke Suzuki 2018-08-16 11:11:43 PDT
Thanks! https://bugs.webkit.org/show_bug.cgi?id=188662