Bug 90400 – Make the skia_test_expectations.txt file optional.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ojan Vafai]

Make the skia_test_expectations.txt file optional.

[Ojan Vafai]

Created attachment 150471 [details]
Patch

[Emil A Eklund]

This is great, thank you!

[Dirk Pranke]

Comment on attachment 150471 [details]
Patch

So, I'm conflicted about this change ... r-/cq-'ing it for a chance at discussion.

The skia test_expectations file was never optional. The chromium downstream file is optional. However, the skia file is used by both the upstream and downstream bots, and corresponds to the version of skia that both bots build against.

In other words, this file always exists and is used on the bots, and thus, it would be an error if the file was missing.

Therefore, if you are trying to figure out what the expectations for a test are, and you don't have this file in your checkout, there's a chance you will form the wrong expectations.

This means that if you run rebaseline-expectations, you better have this file in your checkout. If rebaseline-test is also trying to update the expectations, it needs the skia file as well, and I think that this implies that you need this even if you're running w/ garden-o-matic.

On the other hand, it's obviously annoying to have to do 'update-webkit --chromium' just for this one file, especially when this file is supposed to be empty most of the time.

So, I'm not sure what the right thing to do here is.

[Ojan Vafai]

I think this won't generally be a problem in practice. update-expectations during rebaseline-test might not do the 100% optimal thing in a case where a test is listed in both files, but it won't break anything either, i.e. it'll leave the line in the skia test expectations file when it could have removed it.

I think this is preferable to requiring anyone rebaselining chromium results to need to get the skia test expectations file.

More importantly, most people working in WebKit won't even notice it modifying this file in the chromium repo.

[Dirk Pranke]

Partially I'm concerned because your change doesn't just affect rebaseline-test, it affects *everything* that tries to use a chromium port, and I'm not sure what all the ramifications of that might be.

[Ojan Vafai]

(In reply to comment #5)
> Partially I'm concerned because your change doesn't just affect rebaseline-test, it affects *everything* that tries to use a chromium port, and I'm not sure what all the ramifications of that might be.

Your concern is reasonable. As I see it, this is just some of the cost of having this funky skia test expectations file. I'm not too worried because in order to get to a place where you're actually running the tests locally, you'll necessarily have to checkout this file.

[Ojan Vafai]

Would you be happier if we logged a warning?

[Dirk Pranke]

Comment on attachment 150471 [details]
Patch

(In reply to comment #7)
> Would you be happier if we logged a warning?

Probably :)

Given that I'm not sure what'll break, and you think we should be okay in practice, I'm okay with landing this and we can see what happens. I just wanted people to understand why this was the way it was.

[Ojan Vafai]

Committed r121784: <http://trac.webkit.org/changeset/121784>