Bug 105380 – [chromium] webkitpy-test: executive.py stringify_args error on the release test bot

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

[noel gordon]

http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/32021/steps/webkitpy-test/logs/stdio

Suppressing most webkitpy logging while running unit tests.
Skipping tests in the following modules or packages because they are really, really, slow:
    webkitpy.common.checkout.scm.scm_unittest
    (https://bugs.webkit.org/show_bug.cgi?id=31818; use --all to include)

Skipping tests in the following modules or packages because they fail horribly on win32:
    webkitpy.common.checkout
    webkitpy.common.config
    webkitpy.tool
    (https://bugs.webkit.org/show_bug.cgi?id=54526; use --all to include)

11Checking autoinstalled packages ...
Checking imports ...
Finding the individual test methods ...
Running the tests ...
[111/1309] webkitpy.common.system.executive_unittest.ExecutiveTest.test_run_command_with_unicode erred:
  Traceback (most recent call last):
    File "E:\google-windows-4\chromium-win-release-tests\build\Tools\Scripts\webkitpy\common\system\executive_unittest.py", line 148, in test_run_command_with_unicode
      output = executive.run_command(command_line('echo', unicode_tor_input))
    File "E:\google-windows-4\chromium-win-release-tests\build\Tools\Scripts\webkitpy\common\system\executive.py", line 419, in run_command
      close_fds=self._should_close_fds())
    File "E:\google-windows-4\chromium-win-release-tests\build\Tools\Scripts\webkitpy\common\system\executive.py", line 489, in popen
      string_args = self._stringify_args(args)
    File "E:\google-windows-4\chromium-win-release-tests\build\Tools\Scripts\webkitpy\common\system\executive.py", line 476, in _stringify_args
      string_args = map(unicode, args)
  UnicodeDecodeError: 'ascii' codec can't decode byte 0xf8 in position 23: ordinal not in range(128)
  
[904/1309] webkitpy.layout_tests.run_webkit_tests_integrationtest.RunTest.test_run_part passed
Ran 1309 tests in 17.675s
FAILED (failures=0, errors=1)
program finished with exit code 1
elapsedTime=19.594000

[Eric Seidel (no email)]

I guess I'm not surprised by this.  But it means we need to fix:
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/system/executive.py#L469

presumably to have a fallback character when the encoding fails.

This is basically a real bug which we've had, but are now finally testing. :)  This is just telling us that unicode-based contributor names (and any other unicode) currently cause webkit-patch to barf on windows.

[Eric Seidel (no email)]

I believe we just change that line to .encode(encoding, errors="replace") and it should do something sensible.  The unittest will still fail, so we'd have to skip it for windows.  I can't easily fix this w/o a windows machine.  We can just skip the unittest.  Dirk may have other thoughts.

[Dirk Pranke]

Created attachment 180461 [details]
Patch

[Dirk Pranke]

(In reply to comment #2)
> I believe we just change that line to .encode(encoding, errors="replace") and it should do something sensible.  The unittest will still fail, so we'd have to skip it for windows.  I can't easily fix this w/o a windows machine.  We can just skip the unittest.  Dirk may have other thoughts.

No, I don't think we want to do that ... I think it was just a simple bug in your earlier patch where we were dual-encoding things. I've posted a fix and verified it works on both win and mac.

[Eric Seidel (no email)]

Comment on attachment 180461 [details]
Patch

This will cause the args argument of ScriptError to not be encoded.  But maybe that's fine.  It's possible the map(unicode, args) should be separated from the encode(), and I should not have grouped them both in _stringify.  We can map(unicode) all we want, but we can only encode once, and need to do it right before we call out to subprocess.

[Dirk Pranke]

(In reply to comment #5)
> (From update of attachment 180461 [details])
> This will cause the args argument of ScriptError to not be encoded.  But maybe that's fine.  It's possible the map(unicode, args) should be separated from the encode(), and I should not have grouped them both in _stringify.  We can map(unicode) all we want, but we can only encode once, and need to do it right before we call out to subprocess.

Yeah. I would think either we should leave the args alone in ScriptError (thus capturing what was actually passed to run_command, rather than the encoded version), or stringify in the call to the ScriptError constructor. I'd probably vote for the former.

See, and I told you to leave the stringifying alone :).

[Eric Seidel (no email)]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 180461 [details] [details])
> > This will cause the args argument of ScriptError to not be encoded.  But maybe that's fine.  It's possible the map(unicode, args) should be separated from the encode(), and I should not have grouped them both in _stringify.  We can map(unicode) all we want, but we can only encode once, and need to do it right before we call out to subprocess.
> 
> Yeah. I would think either we should leave the args alone in ScriptError (thus capturing what was actually passed to run_command, rather than the encoded version), or stringify in the call to the ScriptError constructor. I'd probably vote for the former.
> 
> See, and I told you to leave the stringifying alone :).

Yes. :)  But now we understand what's going on!

So I think the ideal fix IMO, is to get rid of _stringify_args, and instead manually run map(unicode, args) in the places we need to, and _encode_if_needed in the places we need to.

Your current patch is also fine, and I'll r+ it.  We can also land yours and I can look at "cleaning this up" more (aka breaking windows harder!) later. :)

[Eric Seidel (no email)]

The problem (as I see it), is that I attempted to get too cutesy in code, and ended up conflating two concepts.

1.  The convenience of being able to pass non-string arguments (aka numbers) to run_command, popen, etc. and not have subprocess throw kittens at us.

2.  The necessity of working around sucky windows unicode support by encoding to the local code-page before calling subprocess.

Both of these are really things that I think subprocess should do for us... :)  But I can see why they chose to make it simple/rigid like they did.

[Eric Seidel (no email)]

It's also really dangerous that I now know how to CC a former maintainer of subprocess on webkit bugs!  Sorry Jeffery.  Well, kinda sorry. :)

[Eric Seidel (no email)]

Dirk: It would be nice if we could come up with a way to test this on non-windows...

[Dirk Pranke]

*** Bug 105989 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Comment on attachment 180461 [details]
Patch

Red bots make noel sad.

[WebKit Review Bot]

Comment on attachment 180461 [details]
Patch

Clearing flags on attachment: 180461

Committed r139579: <http://trac.webkit.org/changeset/139579>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

(In reply to comment #12)
> (From update of attachment 180461 [details])
> Red bots make noel sad.


Some green over there would be nice for a change:

http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29?numbuilds=200