WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 48883
executive_unittest relies on echo and cat utilities from coreutils, which are not present on Windows
https://bugs.webkit.org/show_bug.cgi?id=48883
Summary
executive_unittest relies on echo and cat utilities from coreutils, which are...
Adam Roben (:aroben)
Reported
2010-11-02 15:59:49 PDT
executive_unittest relies on echo and cat utilities from coreutils, which are not present on Windows
Attachments
Patch
(14.45 KB, patch)
2010-11-02 16:01 PDT
,
Adam Roben (:aroben)
no flags
Details
Formatted Diff
Diff
Incorporates Eric's comments
(17.50 KB, patch)
2010-11-04 07:38 PDT
,
Adam Roben (:aroben)
eric
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Adam Roben (:aroben)
Comment 1
2010-11-02 16:01:17 PDT
Created
attachment 72756
[details]
Patch
Adam Barth
Comment 2
2010-11-03 12:04:05 PDT
Comment on
attachment 72756
[details]
Patch Thanks
Eric Seidel (no email)
Comment 3
2010-11-03 12:10:29 PDT
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?
Adam Roben (:aroben)
Comment 4
2010-11-03 12:20:56 PDT
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?
Eric Seidel (no email)
Comment 5
2010-11-03 12:24:15 PDT
(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.
Adam Roben (:aroben)
Comment 6
2010-11-03 12:31:42 PDT
(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.
Adam Roben (:aroben)
Comment 7
2010-11-04 07:38:23 PDT
Created
attachment 72942
[details]
Incorporates Eric's comments
WebKit Review Bot
Comment 8
2010-11-04 07:41:53 PDT
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.
Eric Seidel (no email)
Comment 9
2010-11-04 07:50:40 PDT
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?
Adam Roben (:aroben)
Comment 10
2010-11-04 08:08:38 PDT
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.
Adam Roben (:aroben)
Comment 11
2010-11-04 08:27:52 PDT
Committed
r71336
: <
http://trac.webkit.org/changeset/71336
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug