Bug 48883

Created attachment 72756 [details] Patch

Comment on attachment 72756 [details] Patch Thanks

Comment on attachment 72756 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72756&action=review I'm glad you did this, we've needed these for a while. In general looks great. Please see my comments below. > WebKitTools/Scripts/webkitpy/common/system/executive_unittest.py:51 > + executive.run_command(tuple(echo.command_arguments('foo'))) Why tuple() here? > WebKitTools/Scripts/webkitpy/test/cat.py:27 > + return ['python', __file__] + list(args) .extend(args) (or *args) would have worked here too I think. I wonder if we need to get the interpreter path. We're assuming python is in their path (which is probably a valid assumption), but just curious. > WebKitTools/Scripts/webkitpy/test/cat.py:34 > + if sys.platform == 'win32' and hasattr(sys.stdout, 'fileno'): > + import msvcrt > + import os > + msvcrt.setmode(sys.stdout.fileno(), os.O_BINARY) What does this do? Why is it needed? > WebKitTools/Scripts/webkitpy/test/cat.py:38 > + I believe there are supposed to be two blank lines between any blocks at the global scope. > WebKitTools/Scripts/webkitpy/test/echo.py:31 > +def main(args=None): > + if not args: Now here is one instance, where unless you intended to treat [] and None the same, thsi might be a typo. > WebKitTools/Scripts/webkitpy/test/echo.py:37 > + if sys.platform == 'win32' and hasattr(sys.stdout, 'fileno'): > + import msvcrt > + import os > + msvcrt.setmode(sys.stdout.fileno(), os.O_BINARY) Should we share this code somewhere under a nice name? make_stdout_binary? no clue. > WebKitTools/Scripts/webkitpy/test/echo.py:42 > + args[0:1] = [] This removes one item?

Comment on attachment 72756 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72756&action=review >> WebKitTools/Scripts/webkitpy/common/system/executive_unittest.py:51 >> + executive.run_command(tuple(echo.command_arguments('foo'))) > > Why tuple() here? The point of this line is to test what happens when you pass a tuple to run_command. (You can see this by looking at the previous form of the line.) >> WebKitTools/Scripts/webkitpy/test/cat.py:27 >> + return ['python', __file__] + list(args) > > .extend(args) (or *args) would have worked here too I think. > > I wonder if we need to get the interpreter path. We're assuming python is in their path (which is probably a valid assumption), but just curious. .extend(args) would work, but it seems less concise (since I'd have to declare the list, then extend it, then return the result). Getting the interpreter path is probably a good idea. I guess we can cross that bridge when we come to it. >> WebKitTools/Scripts/webkitpy/test/cat.py:34 >> + msvcrt.setmode(sys.stdout.fileno(), os.O_BINARY) > > What does this do? Why is it needed? On Windows, file descriptors can either be in binary mode or in text mode. In text mode, \n is converted to \r\n. In binary mode it is not. (Maybe there are other differences, too.) This code puts sys.stdout into binary mode. It's needed because our tests expect cat not to modify line endings. >> WebKitTools/Scripts/webkitpy/test/cat.py:38 >> + > > I believe there are supposed to be two blank lines between any blocks at the global scope. We use a single blank line to separate standard imports from package imports, even though those are different "blocks". But I'm fine with adding another blank line here. We should modify check-webkit-style to enforce this. (PEP8 just says "Separate top-level function and class definitions with two blank lines.") >> WebKitTools/Scripts/webkitpy/test/echo.py:31 >> + if not args: > > Now here is one instance, where unless you intended to treat [] and None the same, thsi might be a typo. I did not mean to treat [] and None the same. I guess I should change this to "if args is None:" and add a test. >> WebKitTools/Scripts/webkitpy/test/echo.py:37 >> + msvcrt.setmode(sys.stdout.fileno(), os.O_BINARY) > > Should we share this code somewhere under a nice name? make_stdout_binary? no clue. I guess I could stick a function in webkitpy.common.system. Does that sound good? >> WebKitTools/Scripts/webkitpy/test/echo.py:42 >> + args[0:1] = [] > > This removes one item? Yes. I could use del args[0:1] if that would be clearer. Or args = args[1:]. What do you prefer?

(In reply to comment #4) > (From update of attachment 72756 [details]) > We use a single blank line to separate standard imports from package imports, even though those are different "blocks". But I'm fine with adding another blank line here. We should modify check-webkit-style to enforce this. (PEP8 just says "Separate top-level function and class definitions with two blank lines.") I'm happy to follow pep8 here. I'll stop adding an extra blank line before the __main__ stuff then. > >> WebKitTools/Scripts/webkitpy/test/echo.py:37 > >> + msvcrt.setmode(sys.stdout.fileno(), os.O_BINARY) > > > > Should we share this code somewhere under a nice name? make_stdout_binary? no clue. > > I guess I could stick a function in webkitpy.common.system. Does that sound good? I like sharing code. :) So yes. > >> WebKitTools/Scripts/webkitpy/test/echo.py:42 > >> + args[0:1] = [] > > > > This removes one item? > > Yes. I could use del args[0:1] if that would be clearer. Or args = args[1:]. What do you prefer? args = args[1:] or args.pop(0) seem slightly clearer to me.

(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 72756 [details] [details]) > > >> WebKitTools/Scripts/webkitpy/test/echo.py:42 > > >> + args[0:1] = [] > > > > > > This removes one item? > > > > Yes. I could use del args[0:1] if that would be clearer. Or args = args[1:]. What do you prefer? > > args = args[1:] or args.pop(0) seem slightly clearer to me. Turns out "del args[0]" will work. I'll go with that, as it seems clearest of all.

Created attachment 72942 [details] Incorporates Eric's comments

Attachment 72942 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKitTools/ChangeLog', u'WebKitTools/Scripts/webkitpy/common/system/executive_unittest.py', u'WebKitTools/Scripts/webkitpy/common/system/fileutils.py', u'WebKitTools/Scripts/webkitpy/test/cat.py', u'WebKitTools/Scripts/webkitpy/test/cat_unittest.py', u'WebKitTools/Scripts/webkitpy/test/echo.py', u'WebKitTools/Scripts/webkitpy/test/echo_unittest.py']" exit_code: 1 WebKitTools/Scripts/webkitpy/test/echo.py:31: expected 2 blank lines, found 1 [pep8/E302] [5] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 72942 [details] Incorporates Eric's comments View in context: https://bugs.webkit.org/attachment.cgi?id=72942&action=review Looks good. You'll want to fix the style before landing. > WebKitTools/Scripts/webkitpy/common/system/fileutils.py:33 > +def make_stdout_binary(): > + """Puts sys.stdout into binary mode (on platforms that have a distinction > + between text and binary mode).""" > + if sys.platform != 'win32' or not hasattr(sys.stdout, 'fileno'): > + return > + import msvcrt > + import os > + msvcrt.setmode(sys.stdout.fileno(), os.O_BINARY) Do we need a way to reset out of binary mode after unit tests? Is it a concern that binary mode may bleed between unit tests? I guess if we're using OutputCapture it's not? > WebKitTools/Scripts/webkitpy/test/cat.py:34 > + if sys.platform == 'win32' and hasattr(sys.stdout, 'fileno'): > + import msvcrt > + import os > + msvcrt.setmode(sys.stdout.fileno(), os.O_BINARY) So you have a make_stdout_binary call but you're not using it here. :) > WebKitTools/Scripts/webkitpy/test/echo.py:27 > +# Add WebKitTools/Scripts to the path to ensure we can find webkitpy. > +sys.path.append(os.path.dirname(os.path.dirname(os.path.dirname(__file__)))) Why does echo need this and webkitpy does not?

Comment on attachment 72942 [details] Incorporates Eric's comments View in context: https://bugs.webkit.org/attachment.cgi?id=72942&action=review >> WebKitTools/Scripts/webkitpy/common/system/fileutils.py:33 >> + msvcrt.setmode(sys.stdout.fileno(), os.O_BINARY) > > Do we need a way to reset out of binary mode after unit tests? Is it a concern that binary mode may bleed between unit tests? I guess if we're using OutputCapture it's not? Yeah, OutputCapture is saving us here by swapping in a new sys.stdout. I could add a make_stdout_text but we wouldn't need to call it anywhere. >> WebKitTools/Scripts/webkitpy/test/cat.py:34 >> + msvcrt.setmode(sys.stdout.fileno(), os.O_BINARY) > > So you have a make_stdout_binary call but you're not using it here. :) Whoopsie! I'll have to add the sys.path gunk here, too. >> WebKitTools/Scripts/webkitpy/test/echo.py:27 >> +sys.path.append(os.path.dirname(os.path.dirname(os.path.dirname(__file__)))) > > Why does echo need this and webkitpy does not? It looks, when you execute a Python script, the script's parent directory gets put on the front of sys.path. test-webkitpy, by virtue of being in WebKitTools/Scripts, thus gets WebKitTools/Scripts in its sys.path automatically. Since this script is not in that directory, we have to add it manually.

Committed r71336: <http://trac.webkit.org/changeset/71336>