Bug 58196 – new-run-webkit-tests: add unit tests for Port.diff_image()

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

[Dirk Pranke]

new-run-webkit-tests: add unit tests for Port.diff_image()

[Dirk Pranke]

Created attachment 88942 [details]
Patch

[Ojan Vafai]

Comment on attachment 88942 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:123
> +        # Routine is not implemented.

this comment here and below doesn't actually communicate anything more to me that the code.

> Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:135
> +        if not port:
> +            return

i don't get when we would ever return a null port given that this is a unittest. but i see that the other tests all do this, so i won't block this patch on that.

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 88942 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88942&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:123
> > +        # Routine is not implemented.
> 
> this comment here and below doesn't actually communicate anything more to me that the code.
> 

Yeah, in retrospect you're right. I'm not sure what the best way to convey why those routines are no-ops is. 

The base class (PortTestCase) implements a bunch of tests that should be done against any Port implementation, e.g., test_baseline_search_path() tests that each port has implemented baseline_search_path() correctly. The default implementation of baseline_search_path() throws a NotImplementedError, but the ChromiumPort doesn't override it, so test_baseline_search_path() would fail. So, we override the test here. 

Any suggestions on how to summarize that? Or would you say that no summary is necessary?

> > Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:135
> > +        if not port:
> > +            return
> 
> i don't get when we would ever return a null port given that this is a unittest. but i see that the other tests all do this, so i won't block this patch on that.

The original intent of the PortTestCase class was to do a bunch of integration tests (like actually diffing images). In that case, the make_port() routine was allowed to return None if it could not actually function (like, trying to run the windows ImageDiff on a mac). 

I've gradually moved away from this and been adding unit tests that should pass on every port, like these, in which case the port routine should be non-null. I need a better way to indicate which type of test is which and figure out which test should run and which should be skipped.

[Ojan Vafai]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 88942 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=88942&action=review
> > 
> > > Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:123
> > > +        # Routine is not implemented.
> > 
> > this comment here and below doesn't actually communicate anything more to me that the code.
> > 
> 
> Yeah, in retrospect you're right. I'm not sure what the best way to convey why those routines are no-ops is. 
> 
> The base class (PortTestCase) implements a bunch of tests that should be done against any Port implementation, e.g., test_baseline_search_path() tests that each port has implemented baseline_search_path() correctly. The default implementation of baseline_search_path() throws a NotImplementedError, but the ChromiumPort doesn't override it, so test_baseline_search_path() would fail. So, we override the test here. 
> 
> Any suggestions on how to summarize that? Or would you say that no summary is necessary?

Something like: "Override this test since ChromiumPort doesn't implement the APIs necessary to pass it."

Although, it's weird to have a test in the base test class that doesn't pass on all the ports. A couple options:
-Move the test into a subclass
-Implement noop versions of the appropriate methods in ChromiumPort

I'm fine with just adding the comment though.

> > > Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:135
> > > +        if not port:
> > > +            return
> > 
> > i don't get when we would ever return a null port given that this is a unittest. but i see that the other tests all do this, so i won't block this patch on that.
> 
> The original intent of the PortTestCase class was to do a bunch of integration tests (like actually diffing images). In that case, the make_port() routine was allowed to return None if it could not actually function (like, trying to run the windows ImageDiff on a mac). 
> 
> I've gradually moved away from this and been adding unit tests that should pass on every port, like these, in which case the port routine should be non-null. I need a better way to indicate which type of test is which and figure out which test should run and which should be skipped.

Makes sense.

[Dirk Pranke]

(In reply to comment #4)
> 
> Although, it's weird to have a test in the base test class that doesn't pass on all the ports. A couple options:
> -Move the test into a subclass
> -Implement noop versions of the appropriate methods in ChromiumPort
> 

I agree that it's a bit weird; The problem is more that ChromiumPort isn't a real port. It's probably debatable whether it should even be subclassing PortTestCase.

[Dirk Pranke]

Committed r83476: <http://trac.webkit.org/changeset/83476>