| Summary: | Make the skia_test_expectations.txt file optional. | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ojan Vafai <ojan> | ||||
| Component: | New Bugs | Assignee: | Ojan Vafai <ojan> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | abarth, dpranke, eae, epoger, eric, tony, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Ojan Vafai
2012-07-02 13:51:57 PDT
Created attachment 150471 [details]
Patch
This is great, thank you! 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.
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. 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. (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. Would you be happier if we logged a warning? 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. Committed r121784: <http://trac.webkit.org/changeset/121784> |