Bug 47959 – Add support for --tolerance in NRWT

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Mihai Parparita]

Add support for --tolerance in NRWT

[Mihai Parparita]

Created attachment 71234 [details]
Patch

[Dirk Pranke]

Comment on attachment 71234 [details]
Patch

LGTM, but I'm not a reviewer.

[Ojan Vafai]

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.

[Mihai Parparita]

Created attachment 71483 [details]
Patch

[Mihai Parparita]

(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.

[Mihai Parparita]

Comment on attachment 71483 [details]
Patch

Oops, doesn't actually work for --tolerance is 0.

[Mihai Parparita]

Created attachment 71484 [details]
Patch

[Mihai Parparita]

(In reply to comment #6)
> Oops, doesn't actually work for --tolerance is 0.

New patch should fix this.

[Dirk Pranke]

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).

[Mihai Parparita]

Created attachment 71492 [details]
Patch

[Mihai Parparita]

(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?

[Dirk Pranke]

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.)

[Mihai Parparita]

Created attachment 71498 [details]
Patch

[Mihai Parparita]

(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.

[Dirk Pranke]

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.

[WebKit Commit Bot]

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

[Mihai Parparita]

Committed r70277: <http://trac.webkit.org/changeset/70277>

[Mihai Parparita]

Comment on attachment 71498 [details]
Patch

I fixed the unit test failure before committing.