|
|
Created:
5 years, 3 months ago by Xianzhu Modified:
4 years, 7 months ago CC:
blink-reviews, chrishtr Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow text expectation for reftests
Paint invalidation tests need to check
- what are invalidated (as text) to ensure no over invalidations;
- the result of invalidation and paint to ensure
- no under-invalidation;
- we paint the invalidated parts correctly.
We can achieve this with both text and pixel expectations. As reftest
is a better way for pixel expectations, -expected.txt and
-expected.html seem a reasonable combination of expectations.
BUG=524134
Committed: https://crrev.com/aac11b64e3093c4ee54c16cfdb331aa1b2d4c46e
Cr-Commit-Position: refs/heads/master@{#394667}
Patch Set 1 #Patch Set 2 : Full change #Patch Set 3 : Rebase #Patch Set 4 : #Messages
Total messages: 42 (13 generated)
wangxianzhu@chromium.org changed reviewers: + dpranke@chromium.org, joelo@chromium.org
This strikes me as a potentially very confusing thing. Devs may not know which tests are "pure" reftests and which tests are invalidation tests that need this behavior, and we might end up getting weird combinations of baselines as a result. How many tests are we talking about wanting this behavior for? Can you post a sample of what some baselines might look like?
I've found the spv2 tests confusing to debug as well. What do you think of a simpler approach were we just append the invalidations in the html, so we can see both?
On 2015/09/15 20:14:42, Dirk Pranke wrote: > This strikes me as a potentially very confusing thing. Devs may not know which > tests are "pure" reftests and which tests are invalidation tests that need this > behavior, and we might end up getting weird combinations of baselines as a > result. > > How many tests are we talking about wanting this behavior for? Can you post a > sample of what some baselines might look like? All tests under paint/invalidation/spv2 have such requirements. In the future we'll convert more text-based-repaint tests into this type, and may add more new tests. Currently because of the limitation of reftests, we put text expectations [1] in the test itself, but this has the following problems: - the expectation must be updated manually. - no good way to show test failure. For now we dump the test failure as text into the DOM of the test, and they will be shown in the image diff as images. [1] The window.expectedPaintInvalidationObjects array contains text expectation of paint invalidations, e.g. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
Now we prefer a more conservative method in https://codereview.chromium.org/1340423004/.
Message was sent while issue was closed.
On 2015/09/15 20:14:42, Dirk Pranke wrote: > This strikes me as a potentially very confusing thing. Devs may not know which > tests are "pure" reftests and which tests are invalidation tests that need this > behavior, and we might end up getting weird combinations of baselines as a > result. > I think paint invalidation tests are just an example of requirement of both text and ref expectations. I guess there are also other existing tests that the authors had to make them pixel tests just because they need a text expectation besides the ref expectation. IMHO, The combination of text and ref expectations looks reasonable instead of weird, just like the combination of text and pixel expectations. I have tried several other methods. None of them looks as simple as this patch. They either result much manual work for examining results and rebaselines, or complex virtual/physical directories and extra run time (e.g. separating text expectations and ref expectations into different virtual suite directories).
On 2015/09/17 18:02:11, Xianzhu wrote: > On 2015/09/15 20:14:42, Dirk Pranke wrote: > > This strikes me as a potentially very confusing thing. Devs may not know > which > > tests are "pure" reftests and which tests are invalidation tests that need > this > > behavior, and we might end up getting weird combinations of baselines as a > > result. > > > > I think paint invalidation tests are just an example of requirement of both text > and ref expectations. I guess there are also other existing tests that the > authors had to make them pixel tests just because they need a text expectation > besides the ref expectation. IMHO, The combination of text and ref expectations > looks reasonable instead of weird, just like the combination of text and pixel > expectations. > > I have tried several other methods. None of them looks as simple as this patch. > They either result much manual work for examining results and rebaselines, or > complex virtual/physical directories and extra run time (e.g. separating text > expectations and ref expectations into different virtual suite directories). Another benefit of this patch might be that we can convert many tests with text+pixel expectations into reftests.
On 2015/09/17 18:07:23, Xianzhu wrote: > On 2015/09/17 18:02:11, Xianzhu wrote: > > On 2015/09/15 20:14:42, Dirk Pranke wrote: > > > This strikes me as a potentially very confusing thing. Devs may not know > > which > > > tests are "pure" reftests and which tests are invalidation tests that need > > this > > > behavior, and we might end up getting weird combinations of baselines as a > > > result. > > > > > > > I think paint invalidation tests are just an example of requirement of both > text > > and ref expectations. I guess there are also other existing tests that the > > authors had to make them pixel tests just because they need a text expectation > > besides the ref expectation. IMHO, The combination of text and ref > expectations > > looks reasonable instead of weird, just like the combination of text and pixel > > expectations. > > > > I have tried several other methods. None of them looks as simple as this > patch. > > They either result much manual work for examining results and rebaselines, or > > complex virtual/physical directories and extra run time (e.g. separating text > > expectations and ref expectations into different virtual suite directories). > > Another benefit of this patch might be that we can convert many tests with > text+pixel expectations into reftests. It's possible that I'm wrong and being able to support ref + -expected.txt would be a net win. I also don't work on the layout tests that much these days so I don't have a properly vested interest in this. However, I don't think we should just add this in a CL, since this potentially affects how the whole test suite functions (and hence affects the whole team). Maybe you could write up a note discussing the problems you've seen, the various options you've looked at, and this as a proposed solution, and send it to chromium-dev to get other people's feedback, to see if the team as a whole agrees that we should support this?
On 2015/09/17 18:27:21, Dirk Pranke wrote: > On 2015/09/17 18:07:23, Xianzhu wrote: > > On 2015/09/17 18:02:11, Xianzhu wrote: > > > On 2015/09/15 20:14:42, Dirk Pranke wrote: > > > > This strikes me as a potentially very confusing thing. Devs may not know > > > which > > > > tests are "pure" reftests and which tests are invalidation tests that need > > > this > > > > behavior, and we might end up getting weird combinations of baselines as a > > > > result. > > > > > > > > > > I think paint invalidation tests are just an example of requirement of both > > text > > > and ref expectations. I guess there are also other existing tests that the > > > authors had to make them pixel tests just because they need a text > expectation > > > besides the ref expectation. IMHO, The combination of text and ref > > expectations > > > looks reasonable instead of weird, just like the combination of text and > pixel > > > expectations. > > > > > > I have tried several other methods. None of them looks as simple as this > > patch. > > > They either result much manual work for examining results and rebaselines, > or > > > complex virtual/physical directories and extra run time (e.g. separating > text > > > expectations and ref expectations into different virtual suite directories). > > > > Another benefit of this patch might be that we can convert many tests with > > text+pixel expectations into reftests. > > It's possible that I'm wrong and being able to support ref + -expected.txt would > > be a net win. I also don't work on the layout tests that much these days so I > don't have a properly vested interest in this. > > However, I don't think we should just add this in a CL, since this potentially > affects how the whole test suite functions (and hence affects the whole team). > > Maybe you could write up a note discussing the problems you've seen, the > various options you've looked at, and this as a proposed solution, and send it > to > chromium-dev to get other people's feedback, to see if the team as a whole > agrees that we should support this? Thanks for the reply. Will send an Email to blink-dev.
ojan@chromium.org changed reviewers: + ojan@chromium.org
This patch isn't sufficient to implement this behavior. A couple things off the top of my head: 1. NeedsRebaseline/NeedsManualRebaseline lines complain if they are set for reftests. That will need to change if it's one of these reftests. 2. For these reftests, the rebaseline tool will need to learn how to rebaseline them. It might already work, but we'd at least need a test showing that it does the right thing.
On 2015/09/19 03:16:15, ojan wrote: > This patch isn't sufficient to implement this behavior. A couple things off the > top of my head: > 1. NeedsRebaseline/NeedsManualRebaseline lines complain if they are set for > reftests. That will need to change if it's one of these reftests. > 2. For these reftests, the rebaseline tool will need to learn how to rebaseline > them. It might already work, but we'd at least need a test showing that it does > the right thing. I have planned the above as the second step after this CL. Actually I had tried to include rebaseline support in this CL before I decided to make this CL as simple as possible. Does this plan look good? For now all tests that will require this CL are generating platform-independent text expectations, so local --reset-results is enough for them.
I don't think we should allow any test formats that need rebaselining and can't be rebaselined by the usual NeedsRebaseline means. So, fixing that would block actually adding any of these tests. I'm OK with you doing this work in two patches, but I don't really see much benefit in doing so. The other thing this patch needs is tests.
Ptal. - Added NeedsRebaseline support - Added tests
On 2015/09/21 23:21:11, Xianzhu wrote: > Ptal. > > - Added NeedsRebaseline support > - Added tests Please hold off on review. New discussions in paint-dev may lead to avoiding the requirement of this CL.
Message was sent while issue was closed.
wangxianzhu@chromium.org changed reviewers: - joelo@chromium.org
I'm bringing this up because we need better pixel expectations for repaint tests. Ptal.
On 2016/05/13 20:34:30, Xianzhu wrote: > I'm bringing this up because we need better pixel expectations for repaint > tests. > > Ptal. Did you address any of the earlier comments? I'm also still not sure we have consensus that this is a good idea.
On 2016/05/13 21:21:37, Dirk Pranke wrote: > On 2016/05/13 20:34:30, Xianzhu wrote: > > I'm bringing this up because we need better pixel expectations for repaint > > tests. > > > > Ptal. > > Did you address any of the earlier comments? I'm also still not sure we have > consensus that this is a good idea. About #12 and #14: now the patch supports rebaseline of text expectations of ref tests. Tests are also included. About necessity: some points in the discussion in blink-dev@ (https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/ref$20tes...): - The proposal looks reasonable, though there might be alternatives (e.g. unit tests) for some paint invalidation testing purposes; - It's useful not only for paint invalidation tests, but also for existing pixel tests to be converted into ref tests.
ping...
On 2016/05/18 16:56:59, Xianzhu wrote: > ping... Sorry for the delay. It looks like you should really have ojan@ or charrelson@ review this, then, given that I'm just not that close to this stuff these days.
dpranke@chromium.org changed reviewers: + chrishtr@chromium.org
On 2016/05/18 23:48:16, Dirk Pranke wrote: > On 2016/05/18 16:56:59, Xianzhu wrote: > > ping... > > Sorry for the delay. It looks like you should really have ojan@ or charrelson@ > review this, then, given that > I'm just not that close to this stuff these days. Whoops, sorry, chrishtr@, rather :).
chrishtr@chromium.org changed reviewers: + wkorman@chromium.org - ojan@chromium.org
+wkorman for review in place of ojan.
lgtm
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1346673003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1346673003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1346673003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1346673003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/1346673003/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1346673003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1346673003/60001
Message was sent while issue was closed.
Description was changed from ========== Allow text expectation for reftests Paint invalidation tests need to check - what are invalidated (as text) to ensure no over invalidations; - the result of invalidation and paint to ensure - no under-invalidation; - we paint the invalidated parts correctly. We can achieve this with both text and pixel expectations. As reftest is a better way for pixel expectations, -expected.txt and -expected.html seem a reasonable combination of expectations. BUG=524134 ========== to ========== Allow text expectation for reftests Paint invalidation tests need to check - what are invalidated (as text) to ensure no over invalidations; - the result of invalidation and paint to ensure - no under-invalidation; - we paint the invalidated parts correctly. We can achieve this with both text and pixel expectations. As reftest is a better way for pixel expectations, -expected.txt and -expected.html seem a reasonable combination of expectations. BUG=524134 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Allow text expectation for reftests Paint invalidation tests need to check - what are invalidated (as text) to ensure no over invalidations; - the result of invalidation and paint to ensure - no under-invalidation; - we paint the invalidated parts correctly. We can achieve this with both text and pixel expectations. As reftest is a better way for pixel expectations, -expected.txt and -expected.html seem a reasonable combination of expectations. BUG=524134 ========== to ========== Allow text expectation for reftests Paint invalidation tests need to check - what are invalidated (as text) to ensure no over invalidations; - the result of invalidation and paint to ensure - no under-invalidation; - we paint the invalidated parts correctly. We can achieve this with both text and pixel expectations. As reftest is a better way for pixel expectations, -expected.txt and -expected.html seem a reasonable combination of expectations. BUG=524134 Committed: https://crrev.com/aac11b64e3093c4ee54c16cfdb331aa1b2d4c46e Cr-Commit-Position: refs/heads/master@{#394667} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/aac11b64e3093c4ee54c16cfdb331aa1b2d4c46e Cr-Commit-Position: refs/heads/master@{#394667} |