WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
58196
new-run-webkit-tests: add unit tests for Port.diff_image()
https://bugs.webkit.org/show_bug.cgi?id=58196
Summary
new-run-webkit-tests: add unit tests for Port.diff_image()
Dirk Pranke
Reported
2011-04-09 19:47:22 PDT
new-run-webkit-tests: add unit tests for Port.diff_image()
Attachments
Patch
(4.77 KB, patch)
2011-04-09 19:51 PDT
,
Dirk Pranke
ojan
: review+
ojan
: commit-queue-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Dirk Pranke
Comment 1
2011-04-09 19:51:53 PDT
Created
attachment 88942
[details]
Patch
Ojan Vafai
Comment 2
2011-04-10 02:01:57 PDT
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
Comment 3
2011-04-10 14:08:43 PDT
(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
Comment 4
2011-04-10 15:24:43 PDT
(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
Comment 5
2011-04-11 12:25:02 PDT
(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
Comment 6
2011-04-11 12:28:22 PDT
Committed
r83476
: <
http://trac.webkit.org/changeset/83476
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug