| Summary: | add unit test for NRWT's --additional-expectations option | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | epoger | ||||
| Component: | Tools / Tests | Assignee: | Elliot Poger <epoger> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | abarth, dpranke, ojan, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
epoger
2012-05-15 11:00:43 PDT
Created attachment 142007 [details]
Patch
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. Comment on attachment 142007 [details] Patch Rejecting attachment 142007 [details] from commit-queue. epoger@google.com 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. :-) 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. Comment on attachment 142007 [details] Patch Rejecting attachment 142007 [details] from commit-queue. epoger@google.com 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. Comment on attachment 142007 [details] Patch Clearing flags on attachment: 142007 Committed r117168: <http://trac.webkit.org/changeset/117168> All reviewed patches have been landed. Closing bug. |