Bug 89791 – Add support for test_expectations_android.txt for overriding test expecations on the chromium-android branch

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Add support for TestExpectationsAndroid for overriding test expecations on the chromium-android branch

[Adam Barth]

Created attachment 149134 [details]
Patch

[Adam Barth]

The file is actually named test_expectations_android.txt downstream.  I'm going to rename it to TestExpectationsAndroid, but I'd like to do that once I've merged this patch back downstream to avoid having to rename the file downstream at the same time.

[Adam Barth]

Created attachment 149136 [details]
Patch

[Dirk Pranke]

Comment on attachment 149136 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:221
> +        return expectations_files

I would've just had:

# FIXME: remove when chromium-android is fully upstreamed.
def expectations_files(self):   
   return super(ChromiumAndroidPort, self).expectations_files() + [self.path_from_webkit_base(...)]

but this is fine.

[Adam Barth]

Committed r121074: <http://trac.webkit.org/changeset/121074>

[Ojan Vafai]

Comment on attachment 149136 [details]
Patch

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

> LayoutTests/platform/chromium/test_expectations_android.txt:4
> +// This file should remain empty. It is used by the chromium-android port in
> +// its downstream repository to manage test expecations downstream. Rather than
> +// add expectations to this file upstream, please add them to the normal
> +// TestExpectations file with the ANDROID keyword.

This comment is very confusing. The downstream android overwrites this file?

There's something that I'm not understanding about how this all hooks together, but based off this comment alone, I don't see why we need this file at all.

[Adam Barth]

> This comment is very confusing. The downstream android overwrites this file?

Yes.  This file is forked downstream.  It contains a list of which tests fail on the chromium-android branch.  They could maintain this information in the main TestExpectations file, but that creates pain during the merge.  Using a separate overrides file makes the merge easier and deals well with expectation conflicts.

> There's something that I'm not understanding about how this all hooks together, but based off this comment alone, I don't see why we need this file at all.

We need this file because it's referenced by webkitpy (so that we can fully unfork webkitpy).  One alternative is to make webkitpy not crash when a TestExpectations file is missing.  The idea with having the file exist is so that it could contain an explanation of what its role is.  If the explanation is unclear, we should try to make it clearer.

[Ojan Vafai]

(In reply to comment #7)
> > This comment is very confusing. The downstream android overwrites this file?
> 
> Yes.  This file is forked downstream.  It contains a list of which tests fail on the chromium-android branch.  They could maintain this information in the main TestExpectations file, but that creates pain during the merge.  Using a separate overrides file makes the merge easier and deals well with expectation conflicts.

That clarifies. So, is this just a temporary thing while unforking? If so, adding that to the comment in the file would clear up my confusion.

> We need this file because it's referenced by webkitpy (so that we can fully unfork webkitpy).  One alternative is to make webkitpy not crash when a TestExpectations file is missing. 

Yeah. The latter is what we did for the skia TestExpectations file. I'm not sure which one I prefer though.

This isn't a big deal. It mostly just wasn't clear to me what I should do when I'm adding a new test that I know will fail everywhere and needs to be rebaselined everywhere. Normally, I'd add it to each TestExpectations file. The right answer here would be to not worry about Android and let Android folks deal with it?

An alternative that would make more sense to me:
1. Have this be a regular TestExpectations file that people can add stuff to as appropriate
2. Have a different TestExpectations file somewhere else in the android repo that is part of the cascade.

This is basically what the skia TestExpectations file does and there doesn't need to be any confusing explanation in the WebKit tree of why this file is special. The Android side file will need a comment, but only Android people will need to understand the Android-specific process.

[Ojan Vafai]

That all said, do whatever makes sense to you here. I don't feel strongly about any of this.

[Adam Barth]

> It mostly just wasn't clear to me what I should do when I'm adding a new test that I know will fail everywhere and needs to be rebaselined everywhere. Normally, I'd add it to each TestExpectations file. The right answer here would be to not worry about Android and let Android folks deal with it?

Correct.  Until the chromium-android tests are running upstream, contributors to webkit.org do not need to worry about whether tests pass or fail on chromium-android.

> An alternative that would make more sense to me:
> 1. Have this be a regular TestExpectations file that people can add stuff to as appropriate

It's never appropriate for contributors to webkit.org to add anything to the file because chromium-android does not yet run tests upstream.  Once they are able to run tests upstream, they'll use the normal TestExpectations file as normal.

> 2. Have a different TestExpectations file somewhere else in the android repo that is part of the cascade.

We could do that, but it's valuable to be able to change WebKit and the expectations file atomically downstream (e.g., when fixing a test).

> This is basically what the skia TestExpectations file does and there doesn't need to be any confusing explanation in the WebKit tree of why this file is special.

Right, but in the case of Skia, the expectations file is intended to be changed atomically with changes to Skia, which is why it's in the Skia repository.  In this case, the file is intended to be changed atomically on the chromium-android branch.

> The Android side file will need a comment, but only Android people will need to understand the Android-specific process.

The file looks totally different in the chromium-android branch because it's filled with test expectations.

The short answer to your question is you should ignore this file.  It's for use in the chromium-android branch.

[Ojan Vafai]

I see. That all makes sense. I suppose it would be more clear to me if it said something like the following:
// This file should remain empty. A fork of this file is used by the chromium-android port in
// its downstream repository to manage test expecations downstream. Rather than
// add expectations to this file upstream, please add them to the normal
// TestExpectations file with the ANDROID keyword. Once Android is running 
// tests upstream, we will unfork this file.

I think the main thing that confused me is that android was using a forked version of the file. Not being familiar with Android's current state, it wasn't obvious to me how this all fit together. Maybe none of this matters anyways. :)

[Adam Barth]

Before I saw your recent message, I tried to update the text of this comment in Bug 89832.  Shall we continue the conversation there?