Add support for --tolerance in NRWT
Created attachment 71234 [details] Patch
Comment on attachment 71234 [details] Patch LGTM, but I'm not a reviewer.
Comment on attachment 71234 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71234&action=review Is there anyway to test this? Not that the tolerance actually does the right thing, just a test that the code doesn't crash when you pass a tolerance would be enough for me. > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1567 > + "ports may ignore this option)", type="float", default=0.1), I'd prefer if we default to 0 and then allow individual ports to override the tolerance somehow. As it is, each port currently has a different default.
Created attachment 71483 [details] Patch
(In reply to comment #3) > Is there anyway to test this? Not that the tolerance actually does the right thing, just a test that the code doesn't crash when you pass a tolerance would be enough for me. Added a test. > I'd prefer if we default to 0 and then allow individual ports to override the tolerance somehow. As it is, each port currently has a different default. I changed it so that the port's default value is used if the flag is not specified.
Comment on attachment 71483 [details] Patch Oops, doesn't actually work for --tolerance is 0.
Created attachment 71484 [details] Patch
(In reply to comment #6) > Oops, doesn't actually work for --tolerance is 0. New patch should fix this.
Okay, after thinking about this more, I suggest you not land this at all, since you don't need most of this code. All you need is the command line parameter. Once that's set, it's passed to the port in the constructor (as part of the options keyword). diff_image() can then pull that and use it accordingly, and shouldn't even take a tolerance parameter (since it will never vary between tests).
Created attachment 71492 [details] Patch
(In reply to comment #9) > Okay, after thinking about this more, I suggest you not land this at all, since you don't need most of this code. > > All you need is the command line parameter. Once that's set, it's passed to the port in the constructor (as part of the options keyword). diff_image() can then pull that and use it accordingly, and shouldn't even take a tolerance parameter (since it will never vary between tests). Agreed that tolerance is not a per-test setting, so it probably doesn't make sense as a test_arg. How about the new version of the patch?
Comment on attachment 71492 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71492&action=review You might want a test or two in port/webkit_unittest.py that checks that the webkit-specific default is preserved/propagated correctly, as well. > WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:140 > + tolerance = 0.1 self.get_option('tolerance') and add a line to the constructor: self.set_option_default('tolerance', 0.1) That way the correct port-specific value actually gets propagated up into the options. (as an aside, I really need to re-work this, because this will all break if the default= value is set in optparse.make_option, even if we use default=None, I think ... need to test this.)
Created attachment 71498 [details] Patch
(In reply to comment #12) > self.get_option('tolerance') > self.set_option_default('tolerance', 0.1) Dirk and I chatted about this, and I can't use that since options.tolerance is always present (just set to None if the flag is omitted), and get_option just checks for hasattr. Added a FIXME for now, Dirk will be redoing per-port flags separately.
Comment on attachment 71498 [details] Patch patch LGTM. I waffled a bit as to whether the test_tolerance() test really should be in port/base_unittest or port/port_testcase.py, and whether there should be a test in port/webkit_unittest.py that is verifying that the webkit default is being honored, but ultimately decided that those could be deferred to bug 48095 where I revamp this whole mechanism.
Comment on attachment 71498 [details] Patch Rejecting patch 71498 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2 Last 500 characters of output: ebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py", line 133, in diff_image sp = self._diff_image_request(expected_contents, actual_contents) File "/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py", line 140, in _diff_image_request if self._options.tolerance is not None: AttributeError: 'MockOptions' object has no attribute 'tolerance' ---------------------------------------------------------------------- Ran 647 tests in 46.321s FAILED (errors=1) Full output: http://queues.webkit.org/results/4670020
Committed r70277: <http://trac.webkit.org/changeset/70277>
Comment on attachment 71498 [details] Patch I fixed the unit test failure before committing.