Bug 39257 – Chromium: new-run-webkit-httpd fails to setup_mount

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Fumitoshi Ukai]

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

[Fumitoshi Ukai]

Created attachment 56306 [details]
Patch

[Eric Seidel (no email)]

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.

[Fumitoshi Ukai]

Created attachment 56309 [details]
Patch

[Fumitoshi Ukai]

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

[Eric Seidel (no email)]

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.

[Fumitoshi Ukai]

Created attachment 56313 [details]
Patch

[Fumitoshi Ukai]

Created attachment 56314 [details]
Patch

[Eric Seidel (no email)]

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.

[Fumitoshi Ukai]

Created attachment 56322 [details]
Patch

[Fumitoshi Ukai]

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.

[Eric Seidel (no email)]

Comment on attachment 56322 [details]
Patch

OK.

[Fumitoshi Ukai]

Committed r59661: <http://trac.webkit.org/changeset/59661>