Bug 53036 – nrwt: clean up unit tests, add test-mac, test-win, port.unit_test_user()

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

[Dirk Pranke]

nrwt: clean up unit tests, add test-mac, test-win, port.unit_test_user()

[Dirk Pranke]

Created attachment 79954 [details]
Patch

[Dirk Pranke]

Created attachment 79955 [details]
fix bug id in ChangeLog

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Adding reviewers.

[Dirk Pranke]

Created attachment 79962 [details]
use user_mock.MockUser()

[Dirk Pranke]

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.

[Dirk Pranke]

Created attachment 79972 [details]
update w/ review feedback from eric

[Dirk Pranke]

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

[Eric Seidel (no email)]

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

[Tony Chang]

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)?

[Dirk Pranke]

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

[Eric Seidel (no email)]

Comment on attachment 79972 [details]
update w/ review feedback from eric

Seems OK.  Tony had some useful-to-fix-comments.

[Dirk Pranke]

(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)?

[Tony Chang]

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.

[Dirk Pranke]

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

[Dirk Pranke]

Committed r76642: <http://trac.webkit.org/changeset/76642>