Bug 47959 - Add support for --tolerance in NRWT
Summary: Add support for --tolerance in NRWT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mihai Parparita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-19 17:52 PDT by Mihai Parparita
Modified: 2010-10-21 17:06 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.36 KB, patch)
2010-10-19 17:55 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (6.69 KB, patch)
2010-10-21 13:35 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (6.84 KB, patch)
2010-10-21 13:41 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (7.51 KB, patch)
2010-10-21 14:22 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (7.68 KB, patch)
2010-10-21 15:00 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Parparita 2010-10-19 17:52:52 PDT
Add support for --tolerance in NRWT
Comment 1 Mihai Parparita 2010-10-19 17:55:18 PDT
Created attachment 71234 [details]
Patch
Comment 2 Dirk Pranke 2010-10-19 19:58:53 PDT
Comment on attachment 71234 [details]
Patch

LGTM, but I'm not a reviewer.
Comment 3 Ojan Vafai 2010-10-20 08:23:35 PDT
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.
Comment 4 Mihai Parparita 2010-10-21 13:35:08 PDT
Created attachment 71483 [details]
Patch
Comment 5 Mihai Parparita 2010-10-21 13:36:01 PDT
(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 6 Mihai Parparita 2010-10-21 13:38:46 PDT
Comment on attachment 71483 [details]
Patch

Oops, doesn't actually work for --tolerance is 0.
Comment 7 Mihai Parparita 2010-10-21 13:41:24 PDT
Created attachment 71484 [details]
Patch
Comment 8 Mihai Parparita 2010-10-21 13:41:53 PDT
(In reply to comment #6)
> Oops, doesn't actually work for --tolerance is 0.

New patch should fix this.
Comment 9 Dirk Pranke 2010-10-21 13:58:41 PDT
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).
Comment 10 Mihai Parparita 2010-10-21 14:22:19 PDT
Created attachment 71492 [details]
Patch
Comment 11 Mihai Parparita 2010-10-21 14:23:07 PDT
(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 12 Dirk Pranke 2010-10-21 14:41:08 PDT
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.)
Comment 13 Mihai Parparita 2010-10-21 15:00:27 PDT
Created attachment 71498 [details]
Patch
Comment 14 Mihai Parparita 2010-10-21 15:02:52 PDT
(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 15 Dirk Pranke 2010-10-21 15:15:29 PDT
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 16 WebKit Commit Bot 2010-10-21 16:45:52 PDT
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
Comment 17 Mihai Parparita 2010-10-21 17:04:55 PDT
Committed r70277: <http://trac.webkit.org/changeset/70277>
Comment 18 Mihai Parparita 2010-10-21 17:05:59 PDT
Comment on attachment 71498 [details]
Patch

I fixed the unit test failure before committing.