Bug 86642 – Add setting of additional_expectations option to chromium_unittest.test_overrides_and_builder_names()

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

[Elliot Poger]

Add setting of additional_expectations option to chromium_unittest.test_overrides_and_builder_names()

[Elliot Poger]

Created attachment 142282 [details]
Patch

[Elliot Poger]

Dirk- can you please review this small change?

[Dirk Pranke]

Comment on attachment 142282 [details]
Patch

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

looks mostly fine, just a couple of nits.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:296
> +        filesystem.files[chromium_overrides_path] = CHROMIUM_OVERRIDES

please change this to filesystem.write_text_file(chromium_overrides_path, CHROMIUM_OVERRIDES); originally I used to allow settings .files directly, but that turns out to be fragile.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:300
> +        filesystem.files[additional_expectations_path] = ADDITIONAL_EXPECTATIONS

same thing. Also, you probably want to use an absolute path here.

[Elliot Poger]

Comment on attachment 142282 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:296
>> +        filesystem.files[chromium_overrides_path] = CHROMIUM_OVERRIDES
> 
> please change this to filesystem.write_text_file(chromium_overrides_path, CHROMIUM_OVERRIDES); originally I used to allow settings .files directly, but that turns out to be fragile.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:300
>> +        filesystem.files[additional_expectations_path] = ADDITIONAL_EXPECTATIONS
> 
> same thing. Also, you probably want to use an absolute path here.

Done.

[Elliot Poger]

Created attachment 142324 [details]
Patch

[Dirk Pranke]

Comment on attachment 142324 [details]
Patch

for what it's worth, since I R+'ed the previous patch, you didn't need to set R? on this one.

[Elliot Poger]

(In reply to comment #6)
> (From update of attachment 142324 [details])
> for what it's worth, since I R+'ed the previous patch, you didn't need to set R? on this one.

Unfortunately, "webkit-patch upload" set R=? for me. :-(

If I didn't hear back from you, I was gonna land it manually.  Thanks.

[WebKit Review Bot]

Comment on attachment 142324 [details]
Patch

Clearing flags on attachment: 142324

Committed r117443: <http://trac.webkit.org/changeset/117443>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 142324 [details] [details])
> > for what it's worth, since I R+'ed the previous patch, you didn't need to set R? on this one.
> 
> Unfortunately, "webkit-patch upload" set R=? for me. :-(
> 
> If I didn't hear back from you, I was gonna land it manually.  Thanks.

You can actually use the --no-review flag to webkit-patch, or clear the R? manually in the bug.