In chromium, new-run-webkit-httpd fails to setup mount. cf. http://build.chromium.org/buildbot/waterfall/builders/NACL%20Tests/builds/10521/steps/nacl_ui_tests/logs/stdio
Created attachment 56306 [details] Patch
Comment on attachment 56306 [details] Patch LGTM. A unit test would be even better. There is a MockExecutive inside mocktool.py which knows how to log arguments. outputcapture.py can be used to assert that the right output was logged.
Created attachment 56309 [details] Patch
(In reply to comment #2) > (From update of attachment 56306 [details]) > LGTM. A unit test would be even better. There is a MockExecutive inside mocktool.py which knows how to log arguments. outputcapture.py can be used to assert that the right output was logged. Thanks for review. Added a unit test.
Comment on attachment 56309 [details] Patch WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win_unittest.py:47 + expected_stdout="", These default to "", so the explicit ="" are not needed. WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win_unittest.py:51 + orig_platform = sys.platform I would have abstracted that code into some sort of setup/teardown function. If we wanted to get fancy we could use python 2.5+ contextual objects and a with statement, but that's probably overkill. WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win_unittest.py:62 + expected_stdout="", Not needed, defaults to "" LGTM otherwise.
Created attachment 56313 [details] Patch
Created attachment 56314 [details] Patch
Comment on attachment 56314 [details] Patch WebKitTools/Scripts/webkitpy/common/system/executive.py:268 + if not isinstance(args, list) and not isinstance(args, tuple): I think assert(isinstance(args, list) or isinstance(args, tuple)) would be clearer. ScriptError is generally used for when the script being run has a error. WebKitTools/Scripts/webkitpy/common/system/executive_unittest.py:47 + self.assertRaises(ScriptError, executive.run_command, "echo") This would be AssertionError then. WebKitTools/Scripts/webkitpy/common/system/executive_unittest.py:49 + executive.run_command(["echo", "foo"]) Is this going to end up outputing "foo" on the console when test-webkitpy is run? WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win_unittest.py:59 + setup_mount = port.path_from_chromium_base("third_party", This will fail to run in a non-chromium environment. Instead you should mock out path_from_chromium_base for the test, no? r- because the test would fail from a normal webkit checkout.
Created attachment 56322 [details] Patch
Thanks for review! (In reply to comment #8) > (From update of attachment 56314 [details]) > WebKitTools/Scripts/webkitpy/common/system/executive.py:268 > + if not isinstance(args, list) and not isinstance(args, tuple): > I think assert(isinstance(args, list) or isinstance(args, tuple)) would be clearer. ScriptError is generally used for when the script being run has a error. I see. Fixed. > > WebKitTools/Scripts/webkitpy/common/system/executive_unittest.py:47 > + self.assertRaises(ScriptError, executive.run_command, "echo") > This would be AssertionError then. Fixed. > > WebKitTools/Scripts/webkitpy/common/system/executive_unittest.py:49 > + executive.run_command(["echo", "foo"]) > Is this going to end up outputing "foo" on the console when test-webkitpy is run? "foo\n" will be returned from run_command, not to show on console. > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win_unittest.py:59 > + setup_mount = port.path_from_chromium_base("third_party", > This will fail to run in a non-chromium environment. Instead you should mock out path_from_chromium_base for the test, no? Ah. Fixed by using mock_path_from_chromium_base. > r- because the test would fail from a normal webkit checkout. Tested in normal webkit checkout.
Comment on attachment 56322 [details] Patch OK.
Committed r59661: <http://trac.webkit.org/changeset/59661>