nrwt: clean up unit tests, add test-mac, test-win, port.unit_test_user()
Created attachment 79954 [details] Patch
Created attachment 79955 [details] fix bug id in ChangeLog
Comment on attachment 79955 [details] fix bug id in ChangeLog 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.
Adding reviewers.
Created attachment 79962 [details] use user_mock.MockUser()
Sorry, I posted this for review before it was quite ready, so you probably caught some stuff in flux. (In reply to comment #3) > (From update of attachment 79955 [details]) > 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. > I'm not sure which code this is a reference to. If it's the port.unit_test_filesystem() stuff, I'll talk about that below. I've removed port.unit_test_user(). If it's something else, lmk. > > Tools/Scripts/webkitpy/layout_tests/port/test.py:194 > > +class TestUser(): > > Why invent a second shared mock? Why not use MockUser in mocktool.py? > See, I knew there was a MockUser that I'd used before somewhere. I'll switch to that. > > 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? > Good suggestion. > > 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. > I thought I responded to this in another patch somewhere, but you may not have seen it (at least I don't remember seeing a response from you). I understand the concern. I'm not sure what the best way to fix this is. I want the object to live in port/test.py because that's really where it needs to be (alongside the other test code). However, since we export the get() function in port/__init__.py, I don't really like other callers reaching into the port/* package to try and get stuff that isn't otherwise exported. I don't know if there's a better way to solve this, and am certainly open to suggestions. > > 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. Agreed. This is gone now.
Created attachment 79972 [details] update w/ review feedback from eric
(In reply to comment #6) > > > Tools/Scripts/webkitpy/layout_tests/port/test.py:194 > > > +class TestUser(): > > > > Why invent a second shared mock? Why not use MockUser in mocktool.py? > > > > See, I knew there was a MockUser that I'd used before somewhere. I'll switch to that. > I often forget about (and am trying not to use mocktool where I'm not explicitly doing tool-related stuff). Any objection to moving MockUser out into common/system/user_mock.py so that it sits alongside the real User object?
(In reply to comment #8) > (In reply to comment #6) > > > > Tools/Scripts/webkitpy/layout_tests/port/test.py:194 > > > > +class TestUser(): > > > > > > Why invent a second shared mock? Why not use MockUser in mocktool.py? > > > > > > > See, I knew there was a MockUser that I'd used before somewhere. I'll switch to that. > > > > I often forget about (and am trying not to use mocktool where I'm not explicitly doing tool-related stuff). Any objection to moving MockUser out into common/system/user_mock.py so that it sits alongside the real User object? Yeah. You (or someone?) long ago proposed splitting mocktool.py out into mock files which live next to the various files they're mocking. I support such. :)
Comment on attachment 79972 [details] update w/ review feedback from eric View in context: https://bugs.webkit.org/attachment.cgi?id=79972&action=review Eric, did you want to take another pass at this? > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:206 > if self.files[src] is None: > self._raise_not_found(src) > self.files[dst] = self.files[src] > + self.written_files[dst] = self.files[dst] > self.files[src] = None > + self.written_files[src] = None Nit: src -> source and dst -> destination > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:213 > - def open_binary_tempfile(self, suffix): > + def open_binary_tempfile(self, suffix=''): > path = self._mktemp(suffix) > - return WritableFileObject(self, path), path > + return (WritableFileObject(self, path), path) Nit: It would be nice in a follow up patch to just return a WritableFileObject and have a path() getter on WritableFileObject. > Tools/Scripts/webkitpy/layout_tests/port/test.py:207 > + if not port_name or port_name == 'test': > + port_name = 'test-mac' The port_name == 'test' condition seems weird. Can we adjust the callers to pass in test-mac instead of test? > Tools/Scripts/webkitpy/layout_tests/port/test.py:270 > + if self._name == 'test-win': > + return 'win' > return 'mac' Nit: Should this also use a map (like test_platform_name_to_name)?
(In reply to comment #10) > (From update of attachment 79972 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=79972&action=review > > Eric, did you want to take another pass at this? > > > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:206 > > if self.files[src] is None: > > self._raise_not_found(src) > > self.files[dst] = self.files[src] > > + self.written_files[dst] = self.files[dst] > > self.files[src] = None > > + self.written_files[src] = None > > Nit: src -> source and dst -> destination > Will fix. > > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:213 > > - def open_binary_tempfile(self, suffix): > > + def open_binary_tempfile(self, suffix=''): > > path = self._mktemp(suffix) > > - return WritableFileObject(self, path), path > > + return (WritableFileObject(self, path), path) > > Nit: It would be nice in a follow up patch to just return a WritableFileObject and have a path() getter on WritableFileObject. > I can do this, but I'm curious, why do you think that that would be better than this? > > Tools/Scripts/webkitpy/layout_tests/port/test.py:207 > > + if not port_name or port_name == 'test': > > + port_name = 'test-mac' > > The port_name == 'test' condition seems weird. Can we adjust the callers to pass in test-mac instead of test? > Yeah, we can, but I'd prefer to do that in a separate patch. As you surmise, this is largely for compatibility. > > Tools/Scripts/webkitpy/layout_tests/port/test.py:270 > > + if self._name == 'test-win': > > + return 'win' > > return 'mac' > > Nit: Should this also use a map (like test_platform_name_to_name)? This does get changed to that in bug 51222 (two patches on).
Comment on attachment 79972 [details] update w/ review feedback from eric Seems OK. Tony had some useful-to-fix-comments.
(In reply to comment #12) > (From update of attachment 79972 [details]) > Seems OK. Tony had some useful-to-fix-comments. Apart from the src->source change, were any of the others things you think should be changed in this patch before it lands (as opposed to later patches)?
Comment on attachment 79972 [details] update w/ review feedback from eric (In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 79972 [details] [details]) > > Seems OK. Tony had some useful-to-fix-comments. > > Apart from the src->source change, were any of the others things you think should be changed in this patch before it lands (as opposed to later patches)? Follow up patches are fine. I prefer not returning tuples because it's fragile and often hard to read. It also makes it easy to grow the tuple, making things harder to follow. I much prefer just returning a class with named members because declaring classes is so light weight compared to other languages.
(In reply to comment #14) > (From update of attachment 79972 [details]) > (In reply to comment #13) > > (In reply to comment #12) > > > (From update of attachment 79972 [details] [details] [details]) > > > Seems OK. Tony had some useful-to-fix-comments. > > > > Apart from the src->source change, were any of the others things you think should be changed in this patch before it lands (as opposed to later patches)? > > Follow up patches are fine. I prefer not returning tuples because it's fragile and often hard to read. It also makes it easy to grow the tuple, making things harder to follow. > > I much prefer just returning a class with named members because declaring classes is so light weight compared to other languages. Those are good reasons. I would only tend to define a new class if the two returned values are obviously related (as they are in this case).
Committed r76642: <http://trac.webkit.org/changeset/76642>