Bug 187581

Summary: webkitpy.port.server_process_unittest.TestServerProcess.test_basic failed on Windows Python
Product: WebKit Reporter: Fujii Hironori <Hironori.Fujii>
Component: Tools / TestsAssignee: Fujii Hironori <Hironori.Fujii>
Status: RESOLVED FIXED    
Severity: Normal CC: Basuke.Suzuki, bfulgham, dbates, ddkilzer, ews-watchlist, glenn, jbedard, lforschler, pvollan, ross.kirsling, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 187547    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews200 for win-future
none
Patch
none
Archive of layout-test-results from ews206 for win-future
none
Patch to land none

Description Fujii Hironori 2018-07-11 23:46:25 PDT
webkitpy.port.server_process_unittest.TestServerProcess.test_basic failed on Windows Python

While it passes on Cygwin Python, it fails in Windows Python:

> C:\webkit>python Tools\Scripts\test-webkitpy -v webkitpy.port.server_process_unittest
> Skipping lldb_webkit tests; could not find path to lldb.py ''.
> Skipping QueueStatusServer tests; the Google AppEngine Python SDK is not installed.
> Suppressing most webkitpy logging while running unit tests.
> [1/5] webkitpy.port.server_process_unittest.TestServerProcess.test_basic failed:
>   Traceback (most recent call last):
>     File "C:\webkit\Tools\Scripts\webkitpy\port\server_process_unittest.py", line 111, in test_basic
>       self.assertEqual(proc.poll(), 0)
>   AssertionError: None != 0
> 
> [2/5] webkitpy.port.server_process_unittest.TestServerProcess.test_broken_pipe passed
> [3/5] webkitpy.port.server_process_unittest.TestServerProcess.test_cleanup passed
> [4/5] webkitpy.port.server_process_unittest.TestServerProcess.test_process_crashing passed
> [5/5] webkitpy.port.server_process_unittest.TestServerProcess.test_process_crashing_no_data passed
> Ran 5 tests in 0.484s
> FAILED (failures=1, errors=0)
> 
> C:\webkit>
Comment 1 Fujii Hironori 2018-07-11 23:50:24 PDT
This test has been modifed for Windows Python.

Bug 151721 – [Win] Run non-http tests without Cygwin
http://trac.webkit.org/changeset/192944
Comment 2 Fujii Hironori 2018-07-11 23:53:14 PDT
If I remove the condition "sys.platform.startswith('win')", another failure happens.
This means stderr is not ouput.

> C:\webkit>python Tools\Scripts\test-webkitpy -v webkitpy.port.server_process_unittest
> Skipping lldb_webkit tests; could not find path to lldb.py ''.
> Skipping QueueStatusServer tests; the Google AppEngine Python SDK is not installed.
> Suppressing most webkitpy logging while running unit tests.
> [1/5] webkitpy.port.server_process_unittest.TestServerProcess.test_basic erred:
>   Traceback (most recent call last):
>     File "C:\webkit\Tools\Scripts\webkitpy\port\server_process_unittest.py", line 122, in test_basic
>       self.assertEqual(line.strip(), "stderr")
>   AttributeError: 'NoneType' object has no attribute 'strip'
> 
> [2/5] webkitpy.port.server_process_unittest.TestServerProcess.test_broken_pipe passed
> [3/5] webkitpy.port.server_process_unittest.TestServerProcess.test_cleanup passed
> [4/5] webkitpy.port.server_process_unittest.TestServerProcess.test_process_crashing passed
> [5/5] webkitpy.port.server_process_unittest.TestServerProcess.test_process_crashing_no_data passed
> Ran 5 tests in 1.484s
> FAILED (failures=0, errors=1)
Comment 3 Fujii Hironori 2018-07-12 00:20:15 PDT
Created attachment 344831 [details]
Patch
Comment 4 Fujii Hironori 2018-07-12 00:25:40 PDT
All tests will pass after this patch applied both on Windows Python and Cygwin Python.

> C:\webkit>python Tools\Scripts\test-webkitpy -v webkitpy.port.server_process_unittest
> Skipping lldb_webkit tests; could not find path to lldb.py ''.
> Skipping QueueStatusServer tests; the Google AppEngine Python SDK is not installed.
> Suppressing most webkitpy logging while running unit tests.
> [1/5] webkitpy.port.server_process_unittest.TestServerProcess.test_basic passed
> [2/5] webkitpy.port.server_process_unittest.TestServerProcess.test_broken_pipe passed
> [3/5] webkitpy.port.server_process_unittest.TestServerProcess.test_cleanup passed
> [4/5] webkitpy.port.server_process_unittest.TestServerProcess.test_process_crashing passed
> [5/5] webkitpy.port.server_process_unittest.TestServerProcess.test_process_crashing_no_data passed
> Ran 5 tests in 0.639s
> 
> OK
> 
> C:\webkit>
Comment 5 Fujii Hironori 2018-07-12 03:51:39 PDT
WinCairo BuildBots is failing with the same error.

https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Release%20%28Tests%29/builds/559/steps/webkitpy-test/logs/stdio

> [877/1368] webkitpy.port.server_process_unittest.TestServerProcess.test_basic failed:
>   Traceback (most recent call last):
>     File "C:\WebKit-BuildWorker\wincairo-wkl-release-tests\build\Tools\Scripts\webkitpy\port\server_process_unittest.py", line 111, in test_basic
>       self.assertEqual(proc.poll(), 0)
>   AssertionError: None != 0
Comment 6 Basuke Suzuki 2018-07-12 16:36:42 PDT
Comment on attachment 344831 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344831&action=review

> Tools/ChangeLog:19
> +        For failure #2, stdout also needs to be flushed as well as stdin.

Do you mean this?:        For failure #2, stderr also needs to be flushed as well as stdout.
Comment 7 Fujii Hironori 2018-07-13 04:22:49 PDT
Created attachment 344936 [details]
Patch

Thank you for the review, Basuke. Addressed the review feedback.
Comment 8 EWS Watchlist 2018-07-13 11:10:58 PDT
Comment on attachment 344936 [details]
Patch

Attachment 344936 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8526238

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html
Comment 9 EWS Watchlist 2018-07-13 11:11:09 PDT
Created attachment 344958 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 10 Fujii Hironori 2018-07-17 03:33:14 PDT
Could anyone review?
Comment 11 Fujii Hironori 2018-07-18 06:20:18 PDT
Could anyone review?
Comment 12 Daniel Bates 2018-07-18 06:32:11 PDT
Comment on attachment 344936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344936&action=review

> Tools/ChangeLog:23
> +        condition for Windows. Add a new test to check proc.poll() retruns

Retruns => returns

> Tools/Scripts/webkitpy/port/server_process_unittest.py:102
> +        cmd = [sys.executable, '-c', 'import sys; print "stdout"; sys.stdout.flush(); print >>sys.stderr, "stderr"; sys.stderr.flush(); sys.stdin.readline();']

Can you please explain why the flush is needed on Windows? I would have expected stderr to be unbuffered.
Comment 13 Fujii Hironori 2018-07-18 19:08:39 PDT
Comment on attachment 344936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344936&action=review

Thank you for the review.

>> Tools/ChangeLog:23
>> +        condition for Windows. Add a new test to check proc.poll() retruns
> 
> Retruns => returns

Will fix.

>> Tools/Scripts/webkitpy/port/server_process_unittest.py:102
>> +        cmd = [sys.executable, '-c', 'import sys; print "stdout"; sys.stdout.flush(); print >>sys.stderr, "stderr"; sys.stderr.flush(); sys.stdin.readline();']
> 
> Can you please explain why the flush is needed on Windows? I would have expected stderr to be unbuffered.

There are similar questions that stdout is buffered on pipe under Python for Windows.

  Python C program subprocess hangs at "for line in iter" - Stack Overflow
  https://stackoverflow.com/questions/20503671/python-c-program-subprocess-hangs-at-for-line-in-iter
  
  fflush - How fo force subprocess to refresh stdout buffer? - Stack Overflow
  https://stackoverflow.com/questions/22527010/how-fo-force-subprocess-to-refresh-stdout-buffer
  
  buffer - windows console program stdout is buffered when using pipe redirection - Stack Overflow
  https://stackoverflow.com/questions/9037177/windows-console-program-stdout-is-buffered-when-using-pipe-redirection

I found a useful command switch for this issue. I will remake the patch by using this command switch.

https://docs.python.org/2/using/cmdline.html

> -u 
> Force stdin, stdout and stderr to be totally unbuffered.
Comment 14 Fujii Hironori 2018-07-18 19:18:49 PDT
Created attachment 345317 [details]
Patch
Comment 15 EWS Watchlist 2018-07-18 22:49:02 PDT
Comment on attachment 345317 [details]
Patch

Attachment 345317 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8583488

New failing tests:
http/tests/preload/onload_event.html
Comment 16 EWS Watchlist 2018-07-18 22:49:13 PDT
Created attachment 345330 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 17 Fujii Hironori 2018-07-19 18:09:43 PDT
Daniel, could you review again?
Comment 18 Fujii Hironori 2018-07-22 20:50:48 PDT
r?
Comment 19 Daniel Bates 2018-07-22 23:21:58 PDT
Comment on attachment 345317 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345317&action=review

r=me

> Tools/Scripts/webkitpy/port/server_process_unittest.py:102
> +        cmd = [sys.executable, '-uc', 'import sys; print "stdout"; print >>sys.stderr, "stderr"; sys.stdin.readline();']

For your consideration, I suggest adding a comment to explain -u and why we are using -u.

> Tools/Scripts/webkitpy/port/server_process_unittest.py:149
> +        cmd = [sys.executable, '-uc', 'import sys; print "stdout 1"; print "stdout 2"; print "stdout 3"; sys.stdin.readline(); sys.exit(1);']

Ditto.
Comment 20 Fujii Hironori 2018-07-23 00:34:20 PDT
Created attachment 345562 [details]
Patch to land

Thank you for the review and r+
Comment 21 Fujii Hironori 2018-07-23 19:18:38 PDT
Comment on attachment 345562 [details]
Patch to land

Clearing flags on attachment: 345562

Committed r234130: <https://trac.webkit.org/changeset/234130>
Comment 22 Fujii Hironori 2018-07-23 19:18:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Radar WebKit Bug Importer 2018-07-23 19:20:59 PDT
<rdar://problem/42525768>