Bug 74551 – webkitpy: cleanup prior to systemhostifying the layout_test/port* classes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

webkitpy: cleanup prior to systemhostifying the layout_test/port* classes

[Dirk Pranke]

Created attachment 119312 [details]
Patch

[Adam Barth]

Comment on attachment 119312 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119312&action=review

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:57
> +from webkitpy.common.host_mock import MockHost

Generally speaking, production code shouldn't import testing code, including mocks.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:40
> +from webkitpy.common.host_mock import MockHost

Same here.

[Adam Barth]

Dirk, any thoughts on Comment #2?  I noticed you changed the review flag from - to ?, which I took to mean you disagreed with my review, but I didn't see a comment explaining your point of view.

[Dirk Pranke]

(In reply to comment #3)
> Dirk, any thoughts on Comment #2?  I noticed you changed the review flag from - to ?, which I took to mean you disagreed with my review, but I didn't see a comment explaining your point of view.

Oh, that's bizarre. I could have sworn I submitted a comment before setting this back to ?. At any rate, my comment was something along this lines of ...

NRWT was designed intentionally to be able to run --platform test from the command line (to make debugging easier); in order to do that, we need to be able to create a mock host instead of a real host in both the main code and in manager_worker_broker where we're launching subprocesses. We were already doing this in the code; it's just that we were doing this in port/test.py by creating a MockHost inside the Port class. All of this refactoring will make it so that the port classes can't create Hosts (they shouldn't be able to, anyway), so I needed to fix this properly.

I'm open to other ways of getting this functionality, but the only way I can think of is to create a separate test-run-webkit-tests that duplicates this main, and that seems like a silly overhead.

That said, I don't want this particular issue to sideline the rest of this patch, so I can delete those four lines from this patch and split it out into a separate issue if that'll clear up the rest of this.

[Adam Barth]

> That said, I don't want this particular issue to sideline the rest of this patch, so I can delete those four lines from this patch and split it out into a separate issue if that'll clear up the rest of this.

That sounds like a good way to proceed.

[Dirk Pranke]

Created attachment 119492 [details]
remove MockHost() calls from run_webkit_tests.py for now

[Eric Seidel (no email)]

Comment on attachment 119492 [details]
remove MockHost() calls from run_webkit_tests.py for now

OK.

[Dirk Pranke]

Committed r103012: <http://trac.webkit.org/changeset/103012>