[webkit-reviews] review denied: [Bug 53036] nrwt: clean up unit tests, add test-mac, test-win, port.unit_test_user() : [Attachment 79955] fix bug id in ChangeLog

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 53036: nrwt: clean up unit tests, add test-mac, test-win,
port.unit_test_user()
https://bugs.webkit.org/show_bug.cgi?id=53036

Attachment 79955: fix bug id in ChangeLog
https://bugs.webkit.org/attachment.cgi?id=79955&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=79955&action=review

I don't understand the design choice of hanging mocks off the port.

> Tools/Scripts/webkitpy/layout_tests/port/test.py:194
> +class TestUser():

Why invent a second shared mock?  Why not use MockUser in mocktool.py?

> Tools/Scripts/webkitpy/layout_tests/port/test.py:226
> +	   if 'user' not in kwargs or kwargs['user'] is None:

Why not just put user in function declaration as a keword argument with a
default value of None?

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:-59
> -class MockUser():

I see, you moved it.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:88
> +			       user=port.unit_test_user(),
filesystem=filesystem)

I don't like this pattern of the real port objects exposing mocks. 
Historically the real objects knew nothing about the unit tests or mocks.  I
don't understand why we'd want this.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:251
> +	   user = port.unit_test_user()

It might be useful to have a n object which could vend mocks, but I don't see
why that should be the port.  The user object has nothing to do with the port.

The Host object (assuming I landed that patch?), often accessed via self.tool,
holds the user and hte port, etc.  I could see it making sense to have a
MockHost whihc knew how to vend default mock objects.