Bug 49033

Summary: webkitpy.common.system.executive_unittest fails when run with Win32 Python
Product: WebKit Reporter: Adam Roben (:aroben) <aroben>
Component: Tools / TestsAssignee: Adam Roben (:aroben) <aroben>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aroben, cjerdonek, dbates, dpranke, eric, levin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Bug Depends on:    
Bug Blocks: 48728    
Attachments:
Description Flags
Patch
none
Addresses comments from Dave and Eric eric: review+

Description Adam Roben (:aroben) 2010-11-04 15:39:14 PDT
webkitpy.common.system.executive_unittest fails when run with Win32 Python
Comment 1 Adam Roben (:aroben) 2010-11-04 15:41:27 PDT
Created attachment 72991 [details]
Patch
Comment 2 David Levin 2010-11-04 15:50:56 PDT
Comment on attachment 72991 [details]
Patch

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

Minor concern about possibly masking a failure with the roundtripping, but otherwise this looks fine.

> WebKitTools/ChangeLog:37
> +        be replaced by question marks; the round-tripping allows us to expect

It sounds like this is a failure then?
Comment 3 Eric Seidel (no email) 2010-11-04 15:54:54 PDT
Comment on attachment 72991 [details]
Patch

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

In general looks great.  You should consider making your statics non-static and possibly using child_process_encoding from within the unittests themselves.

> WebKitTools/Scripts/webkitpy/common/system/executive.py:242
> +    @staticmethod
> +    def child_process_encoding():

I'm not sure @staticmethod buys us much here.  It just makes methods which depend on these harder to mock/test.

> WebKitTools/Scripts/webkitpy/common/system/executive.py:262
> +        # Win32 Python 2.x uses CreateProcessA rather than CreateProcessW
> +        # to launch subprocesses, so we have to encode arguments using the
> +        # current code page.
> +        return sys.platform in ('cygwin', 'win32')

Do we want to add a python version check here?  We will eventually move to 3, and even just asserting in that case might be helpful.

> WebKitTools/Scripts/webkitpy/common/system/executive.py:264
> +    @classmethod

Likewise here for classmethod.  If you're going to keep this a class method, you should use cls instead of "self" for the arg name.

> WebKitTools/Scripts/webkitpy/common/system/executive.py:286
>          if isinstance(input, unicode):
> -            input = input.encode("utf-8")
> +            input = input.encode(self.child_process_encoding())

I wonder why this unconditionally encodes and run_command_with_teed_output (which is old and mostly deprecated) does not?

> WebKitTools/Scripts/webkitpy/common/system/executive.py:314
> +        args = map(self.encode_input_if_needed, args)

A little odd that this is named encode_input and its being passed "args", but it works.

> WebKitTools/Scripts/webkitpy/common/system/executive_unittest.py:136
> +        if sys.platform == "cygwin":
> +            expected_exit_code = 0  # os.kill results in exit(0) for this process.
> +        elif sys.platform == "win32":
> +            expected_exit_code = 1
>          else:
>              expected_exit_code = -signal.SIGTERM

Hmmm... Seems we might want to use a shared method for this.  expected_child_exit_code_for_kill() or similar.  I feel like I had something like that in an earlier version of the patch where I added this all... and then dropped it.  Now I can't remember why. :)
Comment 4 Adam Roben (:aroben) 2010-11-04 16:01:33 PDT
Comment on attachment 72991 [details]
Patch

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

>> WebKitTools/ChangeLog:37
>> +        be replaced by question marks; the round-tripping allows us to expect
> 
> It sounds like this is a failure then?

Yes, but it's the best we can do given the limitations of Python 2.x on Windows.

>> WebKitTools/Scripts/webkitpy/common/system/executive.py:262
>> +        return sys.platform in ('cygwin', 'win32')
> 
> Do we want to add a python version check here?  We will eventually move to 3, and even just asserting in that case might be helpful.

Sure, I'll add a check.

>> WebKitTools/Scripts/webkitpy/common/system/executive.py:286
>> +            input = input.encode(self.child_process_encoding())
> 
> I wonder why this unconditionally encodes and run_command_with_teed_output (which is old and mostly deprecated) does not?

This unconditionally encoding the string fed to stdin, while the code in _run_command_with_teed_output was conditionally encoding the arguments passed on the command-line. Clearly I need to rename should_encode_child_process_input/encode_input_if_needed to use "arguments" instead of "input".

>> WebKitTools/Scripts/webkitpy/common/system/executive.py:314
>> +        args = map(self.encode_input_if_needed, args)
> 
> A little odd that this is named encode_input and its being passed "args", but it works.

Yeah, I'll change it to "arguments".

>> WebKitTools/Scripts/webkitpy/common/system/executive_unittest.py:136
>>              expected_exit_code = -signal.SIGTERM
> 
> Hmmm... Seems we might want to use a shared method for this.  expected_child_exit_code_for_kill() or similar.  I feel like I had something like that in an earlier version of the patch where I added this all... and then dropped it.  Now I can't remember why. :)

Unfortunately we're getting different exit codes when killing a single process (we get -SIGTERM) vs. killing all processes (we get 0) with Cygwin Python. But I guess we could have two different methods, one for kill and one for kill_all.
Comment 5 Adam Roben (:aroben) 2010-11-04 16:02:37 PDT
(In reply to comment #3)
> (From update of attachment 72991 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72991&action=review
> 
> In general looks great.  You should consider making your statics non-static and possibly using child_process_encoding from within the unittests themselves.

I thought about using child_process_encoding but was worried that it made the test weaker, as if someone introduces a bug in that function then the tests will "expect" that bug. But by not using child_process_encoding I've made the test more fragile. I'm not sure what the best option is.
Comment 6 Eric Seidel (no email) 2010-11-04 16:05:51 PDT
(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 72991 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=72991&action=review
> > 
> > In general looks great.  You should consider making your statics non-static and possibly using child_process_encoding from within the unittests themselves.
> 
> I thought about using child_process_encoding but was worried that it made the test weaker, as if someone introduces a bug in that function then the tests will "expect" that bug. But by not using child_process_encoding I've made the test more fragile. I'm not sure what the best option is.

I think this is what killed my original use of a method for the various exit codes.  IIRC the argument of the reviewer at the time was that my method made my unit testing weaker.

I'm OK with either approach.  I still think we should fight the static cling by removing the @static methods :)  They rarely buy you anything besides untestability in python.
Comment 7 Adam Roben (:aroben) 2010-11-04 16:09:22 PDT
(In reply to comment #6)
> I still think we should fight the static cling by removing the @static methods :)  They rarely buy you anything besides untestability in python.

Sounds fine to me. I'll make them non-static.
Comment 8 David Levin 2010-11-04 16:10:18 PDT
(In reply to comment #4)
> (From update of attachment 72991 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72991&action=review
> 
> >> WebKitTools/ChangeLog:37
> >> +        be replaced by question marks; the round-tripping allows us to expect
> > 
> > It sounds like this is a failure then?
> 
> Yes, but it's the best we can do given the limitations of Python 2.x on Windows.

Would it be possible to use the value of "unicode_tor" on platforms other than Windows rather than roundtripped_tor (since that seems the most trustworthy result and less likely to hide regressions)?
Comment 9 Adam Roben (:aroben) 2010-11-05 05:27:33 PDT
(In reply to comment #8)
> (In reply to comment #4)
> > (From update of attachment 72991 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=72991&action=review
> > 
> > >> WebKitTools/ChangeLog:37
> > >> +        be replaced by question marks; the round-tripping allows us to expect
> > > 
> > > It sounds like this is a failure then?
> > 
> > Yes, but it's the best we can do given the limitations of Python 2.x on Windows.
> 
> Would it be possible to use the value of "unicode_tor" on platforms other than Windows rather than roundtripped_tor (since that seems the most trustworthy result and less likely to hide regressions)?

Yes, I'll do that. Thanks!
Comment 10 Adam Roben (:aroben) 2010-11-05 09:16:40 PDT
Created attachment 73070 [details]
Addresses comments from Dave and Eric
Comment 11 David Levin 2010-11-05 12:03:31 PDT
Comment on attachment 73070 [details]
Addresses comments from Dave and Eric

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

Seems fine to me. (Well, one minor comment.)  It looks like Eric's comments are all addressed as well except for "Seems we might want to use a shared method for this..." which you replied to, but I'd rather let Eric give it one last look in case he had any issues with the response.

> WebKitTools/Scripts/webkitpy/common/system/executive.py:324
> +        if sys.platform == 'win32':

version check here too?
Comment 12 Eric Seidel (no email) 2010-11-05 15:33:42 PDT
Comment on attachment 73070 [details]
Addresses comments from Dave and Eric

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

In general this looks fine.  You probalby want a version check where dave noted.

> WebKitTools/Scripts/webkitpy/common/system/executive.py:341
> +        if sys.platform == 'win32' and versioning.compare_version(sys, '3.0')[0] < 0:

Seems like Chris's versioning module is a bit hard to use.  The whole neeing to grab [0]  and compare it < 0, seems really awkward.

We should probably make his module better, eventually.
Comment 13 Dirk Pranke 2010-11-05 15:41:03 PDT
(In reply to comment #12)
> (From update of attachment 73070 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73070&action=review
> 
> In general this looks fine.  You probalby want a version check where dave noted.
> 
> > WebKitTools/Scripts/webkitpy/common/system/executive.py:341
> > +        if sys.platform == 'win32' and versioning.compare_version(sys, '3.0')[0] < 0:
> 
> Seems like Chris's versioning module is a bit hard to use.  The whole neeing to grab [0]  and compare it < 0, seems really awkward.
> 
> We should probably make his module better, eventually.

I was thinking this as well. It's useful in some cases, but a wrapper that just did a cmp() would be nice.
Comment 14 Adam Roben (:aroben) 2010-11-08 11:16:28 PST
Committed r71547: <http://trac.webkit.org/changeset/71547>