Bug 49033 – webkitpy.common.system.executive_unittest fails when run with Win32 Python

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

[Adam Roben (:aroben)]

webkitpy.common.system.executive_unittest fails when run with Win32 Python

[Adam Roben (:aroben)]

Created attachment 72991 [details]
Patch

[David Levin]

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?

[Eric Seidel (no email)]

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. :)

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

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

[Eric Seidel (no email)]

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

[Adam Roben (:aroben)]

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

[David Levin]

(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)?

[Adam Roben (:aroben)]

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

[Adam Roben (:aroben)]

Created attachment 73070 [details]
Addresses comments from Dave and Eric

[David Levin]

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?

[Eric Seidel (no email)]

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.

[Dirk Pranke]

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

[Adam Roben (:aroben)]

Committed r71547: <http://trac.webkit.org/changeset/71547>