new-run-webkit-tests: add unit tests for Port.diff_image()
Created attachment 88942 [details] Patch
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.
(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.
(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.
(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.
Committed r83476: <http://trac.webkit.org/changeset/83476>