Bug 86501 – add unit test for NRWT's --additional-expectations option

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

This is in addition to the integration test already added in http://trac.webkit.org/changeset/114559

Created attachment 142007 [details]
Patch

[Ojan Vafai]

Comment on attachment 142007 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142007&action=review

tests++

> Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:296
> +            '/tmp/additional-expectations-1.txt']
> +        self.assertEquals('content1\n', port.test_expectations_overrides())
> +
> +        port._options.additional_expectations = [
> +            '/tmp/nonexistent-file', '/tmp/additional-expectations-1.txt']

It surprises me that we don't raise an error when you pass in a non-existant file. Is there a use-case for that? 

Obviously, this patch is fine since it's just testing the existing behavior.

Comment on attachment 142007 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142007&action=review

>> Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:296
>> +            '/tmp/nonexistent-file', '/tmp/additional-expectations-1.txt']
> 
> It surprises me that we don't raise an error when you pass in a non-existant file. Is there a use-case for that? 
> 
> Obviously, this patch is fine since it's just testing the existing behavior.

I just happened upon this behavior... but I can think of at least one potential use case:  I want to add an expectations override file within the Skia tree that NRWT always observes, and depending on a given checkout's DEPS, that expectations override file might or might not be found...

But I'm sure there are other ways to handle that.  I would not be opposed to failing on file-not-found.

[WebKit Review Bot]

Comment on attachment 142007 [details]
Patch

Rejecting attachment 142007 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

Please, nobody hit commit+ on this... I'm supposedly a webkit committer now, so I want to make sure I can get this to work. :-)

[Dirk Pranke]

Comment on attachment 142007 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142007&action=review

>>> Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:296
>>> +            '/tmp/nonexistent-file', '/tmp/additional-expectations-1.txt']
>> 
>> It surprises me that we don't raise an error when you pass in a non-existant file. Is there a use-case for that? 
>> 
>> Obviously, this patch is fine since it's just testing the existing behavior.
> 
> I just happened upon this behavior... but I can think of at least one potential use case:  I want to add an expectations override file within the Skia tree that NRWT always observes, and depending on a given checkout's DEPS, that expectations override file might or might not be found...
> 
> But I'm sure there are other ways to handle that.  I would not be opposed to failing on file-not-found.

The code logs a warning, but intentionally does not fail. At least until proven otherwise, it didn't seem like this should be a fatal error.

There are actually a couple of tests for this functionality in run_webkit_tests_integrationtest.py, but I'm not opposed to adding a unit test here as well.

[WebKit Review Bot]

Comment on attachment 142007 [details]
Patch

Rejecting attachment 142007 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 142007 [details]
Patch

Clearing flags on attachment: 142007

Committed r117168: <http://trac.webkit.org/changeset/117168>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.