Bug 49033

Created attachment 72991 [details] Patch

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

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

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

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

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

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

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

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

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

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