Bug 46854

Summary: new-run-webkit-tests: the Google Chrome port should be able to have its own test expectations overrides
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: Tools / TestsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, tkent, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch tony: review+

Description Dirk Pranke 2010-09-29 16:48:35 PDT
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.
Comment 1 Dirk Pranke 2010-09-29 17:05:07 PDT
Created attachment 69281 [details]
Patch
Comment 2 Tony Chang 2010-09-29 17:41:23 PDT
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?
Comment 3 Dirk Pranke 2010-09-29 17:51:24 PDT
(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?
Comment 4 Tony Chang 2010-09-29 17:58:00 PDT
(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.
Comment 5 Kent Tamura 2010-09-29 18:17:06 PDT
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').
Comment 6 Dirk Pranke 2010-09-29 18:35:25 PDT
(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.
Comment 7 Dirk Pranke 2010-09-29 18:36:05 PDT
Created attachment 69292 [details]
Patch
Comment 8 Dirk Pranke 2010-09-29 18:38:22 PDT
(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.
Comment 9 Tony Chang 2010-09-30 10:31:40 PDT
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.
Comment 10 Dirk Pranke 2010-10-04 15:11:29 PDT
Committed r69040: <http://trac.webkit.org/changeset/69040>
Comment 11 WebKit Review Bot 2010-10-04 15:25:51 PDT
http://trac.webkit.org/changeset/69040 might have broken Chromium Linux Release