Bug 46854 – new-run-webkit-tests: the Google Chrome port should be able to have its own test expectations overrides

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

[Dirk Pranke]

It's possible that an official build of Google Chrome will have different test expectations as well as different baselines. We should be able to support this using just an overrides file in the Chromium repository, since (a) the file will usually be empty, and (b) it's branched at the same times as the official build.

[Dirk Pranke]

Created attachment 69281 [details]
Patch

[Tony Chang]

Comment on attachment 69281 [details]
Patch

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

r- because of unittests, but the approach seems fine.

> WebKitTools/Scripts/webkitpy/layout_tests/port/google_chrome.py:39
> +    except AssertionError:
> +        # FIXME: when does this get raised?

This probably gets raised in a pure webkit checkout which doesn't have a chromium base (I think path_from_chromium_base raises it).

> WebKitTools/Scripts/webkitpy/layout_tests/port/google_chrome_unittest.py:54
> +        # FIXME: make this more robust when we have the Tree() abstraction.
> +        # we should be able to test for the files existing or not, and

I don't like this test because it uses the current checked in files.  Can you just call _test_expectations_overrides directly and pass in mock objects instead?

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 69281 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69281&action=review
> 
> r- because of unittests, but the approach seems fine.
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/google_chrome.py:39
> > +    except AssertionError:
> > +        # FIXME: when does this get raised?
> 
> This probably gets raised in a pure webkit checkout which doesn't have a chromium base (I think path_from_chromium_base raises it).
> 

Yeah, that was what I thought, too, but it looks like tkent took out that AssertionError in r60427 when he added support for DRT. Which I think is actually incorrect, because it assumes you've done build-webkit --chromium to pull in the third_party tree. Adding tkent to confirm ...

http://trac.webkit.org/changeset/60427

> > WebKitTools/Scripts/webkitpy/layout_tests/port/google_chrome_unittest.py:54
> > +        # FIXME: make this more robust when we have the Tree() abstraction.
> > +        # we should be able to test for the files existing or not, and
> 
> I don't like this test because it uses the current checked in files.  Can you just call _test_expectations_overrides directly and pass in mock objects instead?

I agree that the current test isn't great; that's why I put in the FIXME. We can use "proper" mocks once the Tree() abstraction is introduced (see https://bugs.webkit.org/show_bug.cgi?id=46776 , which is currently waiting on a review). Until then, _test_expectations_overrides doesn't take any mock objects, so I'd either have to mock codecs.open(), or change the signature of the file to inject the dependencies to make them testable, and I'm not a big fan of that pattern.

So, I can either mock codecs.open(), or just wait for the other patch to land. I'd prefer to do the latter; what do you think?

[Tony Chang]

(In reply to comment #3)
> I agree that the current test isn't great; that's why I put in the FIXME. We can use "proper" mocks once the Tree() abstraction is introduced (see https://bugs.webkit.org/show_bug.cgi?id=46776 , which is currently waiting on a review). Until then, _test_expectations_overrides doesn't take any mock objects, so I'd either have to mock codecs.open(), or change the signature of the file to inject the dependencies to make them testable, and I'm not a big fan of that pattern.
> 
> So, I can either mock codecs.open(), or just wait for the other patch to land. I'd prefer to do the latter; what do you think?

You can pass in a mock |port| with a mock |path_from_chromium_base| method that returns a test file (or maybe even a StringIO).  It would still touch disk, but at least it would be a test file.

[Kent Tamura]

Comment on attachment 69281 [details]
Patch

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

>>> WebKitTools/Scripts/webkitpy/layout_tests/port/google_chrome.py:39
>>> +        # FIXME: when does this get raised?
>> 
>> This probably gets raised in a pure webkit checkout which doesn't have a chromium base (I think path_from_chromium_base raises it).
> 
> Yeah, that was what I thought, too, but it looks like tkent took out that AssertionError in r60427 when he added support for DRT. Which I think is actually incorrect, because it assumes you've done build-webkit --chromium to pull in the third_party tree. Adding tkent to confirm ...
> 
> http://trac.webkit.org/changeset/60427

I'm not sure whether we should raise AssertionError in path_from_chromium_base or not.  If we'd like to raise it, we should check existence of os.path.join(self._chromium_base_dir, 'webkit').

[Dirk Pranke]

(In reply to comment #5)
> (From update of attachment 69281 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69281&action=review
> 
> >>> WebKitTools/Scripts/webkitpy/layout_tests/port/google_chrome.py:39
> >>> +        # FIXME: when does this get raised?
> >> 
> >> This probably gets raised in a pure webkit checkout which doesn't have a chromium base (I think path_from_chromium_base raises it).
> > 
> > Yeah, that was what I thought, too, but it looks like tkent took out that AssertionError in r60427 when he added support for DRT. Which I think is actually incorrect, because it assumes you've done build-webkit --chromium to pull in the third_party tree. Adding tkent to confirm ...
> > 
> > http://trac.webkit.org/changeset/60427
> 
> I'm not sure whether we should raise AssertionError in path_from_chromium_base or not.  If we'd like to raise it, we should check existence of os.path.join(self._chromium_base_dir, 'webkit').

Well, as long as it's possible to try and create a chromium port and the needed fiiles might not be there, something should assert somewhere. It seems reasonable for path_from_chromium_base to assert if it can't actually find the checkout. It might also be reasonable to fail during the constructor if it figures out that it's not in a checkout.

[Dirk Pranke]

Created attachment 69292 [details]
Patch

[Dirk Pranke]

(In reply to comment #4)
> (In reply to comment #3)
> > I agree that the current test isn't great; that's why I put in the FIXME. We can use "proper" mocks once the Tree() abstraction is introduced (see https://bugs.webkit.org/show_bug.cgi?id=46776 , which is currently waiting on a review). Until then, _test_expectations_overrides doesn't take any mock objects, so I'd either have to mock codecs.open(), or change the signature of the file to inject the dependencies to make them testable, and I'm not a big fan of that pattern.
> > 
> > So, I can either mock codecs.open(), or just wait for the other patch to land. I'd prefer to do the latter; what do you think?
> 
> You can pass in a mock |port| with a mock |path_from_chromium_base| method that returns a test file (or maybe even a StringIO).  It would still touch disk, but at least it would be a test file.

creating a dummy file that has to be checked in and hitting disk to verify it seems like a worse solution than mocking codecs.open and os.path.exists. I've added a patch that does the latter instead.

[Tony Chang]

Comment on attachment 69292 [details]
Patch

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

LGTM with the try/catch removed.

> WebKitTools/Scripts/webkitpy/layout_tests/port/google_chrome.py:40
> +    except AssertionError:
> +        # FIXME: this doesn't seem to get raised after r60427, but it probably
> +        # should still be possible.

If it's not possible for this to be raised, we should remove it for now.  Once we add back the AssertionError, we can put this code back.

[Dirk Pranke]

Committed r69040: <http://trac.webkit.org/changeset/69040>

[WebKit Review Bot]

http://trac.webkit.org/changeset/69040 might have broken Chromium Linux Release