Let repaint tests test pixels by default

Let repaint tests test pixels by default

Text-based outputs are hard to understand and find problems.

Though tile-based re-rasterization may hide some under-invalidations
in pixel result, it can still show some bad rendering corruptions.

- Renamed (fast/repaint/resources/)text-based-repaint.js to
  repaint.js.
- Merged runRepaintTest() and runRepaintAndPixelTest().

It will increase the time to run layout tests a bit. On my Linux
machine, the time to run about 450 fast/repaint tests with one
process increased from about 35s to about 55s. This seems not a
big deal for bots running the layout tests in parallel.

BUG=611462

[Xianzhu, 2016-05-13 00:31:25]

Description was changed from

==========
Let repaint tests test pixels by default

Text-based outputs are hard to understand and find problems.

Though tile-based re-rasterization may hide some under-invalidations
in pixel result, it can still show some bad rendering corruptions.

BUG=611462
==========

to

==========
Let repaint tests test pixels by default

Text-based outputs are hard to understand and find problems.

Though tile-based re-rasterization may hide some under-invalidations
in pixel result, it can still show some bad rendering corruptions.

- Renamed (fast/repaint/resources/)text-based-repaint.js to
  repaint.js.
- Merged runRepaintTest() and runRepaintAndPixelTest().

It will increase the time to run layout tests a bit. On my Linux
machine, the time to run about 450 fast/repaint tests with one
process increased from about 35s to about 55s. This seems not a
big deal for bots running the layout tests in parallel.

BUG=611462
==========

[Xianzhu, 2016-05-13 00:35:32]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, kbr@chromium.org

[Xianzhu, 2016-05-13 00:35:38]

Warning: The CL is big and will be very slow to open.

You can just review by reading the diff of key files through the following
links:

https://codereview.chromium.org/1972273002/diff/40001/third_party/WebKit/Layo...

https://codereview.chromium.org/1972273002/diff/40001/third_party/WebKit/Layo...

I plan to generate the missing pixel expectations with a manual rebaseline after
this CL is landed and passed on all waterfall bots. Not marking them
NeedsRebaseline or NeedsManualRebaseline because they may hide new failures of
so many tests before the rebaseline.

[Ken Russell (switch to Gerrit), 2016-05-13 01:33:50]

kbr@chromium.org changed reviewers:
+ esprehn@chromium.org

[Ken Russell (switch to Gerrit), 2016-05-13 01:33:53]

Thanks for thinking about how to catch these kinds of errors more categorically
Xianzhu.

I'd like to defer review to esprehn@ who will have better opinions than mine of
the structure of the layout tests. Also CC'ing dpranke@.

[Ken Russell (switch to Gerrit), 2016-05-13 01:34:36]

kbr@chromium.org changed reviewers:
- kbr@chromium.org

[chrishtr, 2016-05-13 15:43:06]

https://codereview.chromium.org/1972273002/diff/50001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/repaint/resources/repaint.js (right):

https://codereview.chromium.org/1972273002/diff/50001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/repaint/resources/repaint.js:6: if
(window.testRunner)
Pixel testing is expensive. It also leads to the requirement to rebaseline
pixels for platform-specific expectations.

Instead, I suggest we add some specific ones to test common scenarios such as
the one that led to issue 611462 being
filed.

[Xianzhu, 2016-05-13 16:31:39]

On 2016/05/13 15:43:06, chrishtr wrote:
>
https://codereview.chromium.org/1972273002/diff/50001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/fast/repaint/resources/repaint.js (right):
> 
>
https://codereview.chromium.org/1972273002/diff/50001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/fast/repaint/resources/repaint.js:6: if
> (window.testRunner)
> Pixel testing is expensive. It also leads to the requirement to rebaseline
> pixels for platform-specific expectations.
> 
> Instead, I suggest we add some specific ones to test common scenarios such as
> the one that led to issue 611462 being
> filed.

The problem is that we didn't know the bug before it was filed (though I could
avoid it if I was more careful or discover it with other means) and adding test
coverage afterwards may be inadequate to prevent future errors like this.

To avoid platform-specific expectations I can also convert the tests into ref
tests, but they will cost twice as pixel tests, and for now we don't support
both text and ref expectations. However this is feasible. I've done this for
paint/invalidation/spv2, with separated text and ref tests for text and ref
expectations, respectively. I can pursue to add support of both text and ref
expectations for layout tests to avoid the separated tests. Wdyt about this?

[chrishtr, 2016-05-13 16:49:29]

On 2016/05/13 at 16:31:39, wangxianzhu wrote:
> On 2016/05/13 15:43:06, chrishtr wrote:
> >
https://codereview.chromium.org/1972273002/diff/50001/third_party/WebKit/Layo...
> > File third_party/WebKit/LayoutTests/fast/repaint/resources/repaint.js
(right):
> > 
> >
https://codereview.chromium.org/1972273002/diff/50001/third_party/WebKit/Layo...
> > third_party/WebKit/LayoutTests/fast/repaint/resources/repaint.js:6: if
> > (window.testRunner)
> > Pixel testing is expensive. It also leads to the requirement to rebaseline
> > pixels for platform-specific expectations.
> > 
> > Instead, I suggest we add some specific ones to test common scenarios such
as
> > the one that led to issue 611462 being
> > filed.
> 
> The problem is that we didn't know the bug before it was filed (though I could
avoid it if I was more careful or discover it with other means) and adding test
coverage afterwards may be inadequate to prevent future errors like this.

The tests did provide coverage, we just didn't notice it properly. Adding a ton
of pixel expectations seems a high price to pay.

An interesting alternative: provide a simple script to test whether a patch
caused a pixel difference in one of these tests. Then add
debug output to them instructing developers to run the script if there were any
difference in repaints.

> 
> To avoid platform-specific expectations I can also convert the tests into ref
tests, but they will cost twice as pixel tests, and for now we don't support
both text and ref expectations. However this is feasible. I've done this for
paint/invalidation/spv2, with separated text and ref tests for text and ref
expectations, respectively. I can pursue to add support of both text and ref
expectations for layout tests to avoid the separated tests. Wdyt about this?

[Ken Russell (switch to Gerrit), 2016-05-13 17:26:38]

On 2016/05/13 16:49:29, chrishtr wrote:
> On 2016/05/13 at 16:31:39, wangxianzhu wrote:
> > On 2016/05/13 15:43:06, chrishtr wrote:
> > >
>
https://codereview.chromium.org/1972273002/diff/50001/third_party/WebKit/Layo...
> > > File third_party/WebKit/LayoutTests/fast/repaint/resources/repaint.js
> (right):
> > > 
> > >
>
https://codereview.chromium.org/1972273002/diff/50001/third_party/WebKit/Layo...
> > > third_party/WebKit/LayoutTests/fast/repaint/resources/repaint.js:6: if
> > > (window.testRunner)
> > > Pixel testing is expensive. It also leads to the requirement to rebaseline
> > > pixels for platform-specific expectations.
> > > 
> > > Instead, I suggest we add some specific ones to test common scenarios such
> as
> > > the one that led to issue 611462 being
> > > filed.
> > 
> > The problem is that we didn't know the bug before it was filed (though I
could
> avoid it if I was more careful or discover it with other means) and adding
test
> coverage afterwards may be inadequate to prevent future errors like this.
> 
> The tests did provide coverage, we just didn't notice it properly. Adding a
ton
> of pixel expectations seems a high price to pay.
> 
> An interesting alternative: provide a simple script to test whether a patch
> caused a pixel difference in one of these tests. Then add
> debug output to them instructing developers to run the script if there were
any
> difference in repaints.

It seems likely to me that this instruction will be ignored at some point. Some
sort of test that has pixel output by default is needed to make it obvious to
authors what went wrong. I would recommend either adding a couple of targeted
pixel tests for this failure (under-invalidation?), or converting just a few of
the repaint tests to pixel tests.

> > To avoid platform-specific expectations I can also convert the tests into
ref
> tests, but they will cost twice as pixel tests, and for now we don't support
> both text and ref expectations. However this is feasible. I've done this for
> paint/invalidation/spv2, with separated text and ref tests for text and ref
> expectations, respectively. I can pursue to add support of both text and ref
> expectations for layout tests to avoid the separated tests. Wdyt about this?

Maybe converting a couple of the repaint tests to reftests would be sufficient?

I wouldn't constrain solutions based on machine capacity. The layout tests
should be sharded and swarmed, and more machines can be added.

[Xianzhu, 2016-05-13 17:39:02]

On 2016/05/13 16:49:29, chrishtr wrote:
> On 2016/05/13 at 16:31:39, wangxianzhu wrote:
> > On 2016/05/13 15:43:06, chrishtr wrote:
> > >
>
https://codereview.chromium.org/1972273002/diff/50001/third_party/WebKit/Layo...
> > > File third_party/WebKit/LayoutTests/fast/repaint/resources/repaint.js
> (right):
> > > 
> > >
>
https://codereview.chromium.org/1972273002/diff/50001/third_party/WebKit/Layo...
> > > third_party/WebKit/LayoutTests/fast/repaint/resources/repaint.js:6: if
> > > (window.testRunner)
> > > Pixel testing is expensive. It also leads to the requirement to rebaseline
> > > pixels for platform-specific expectations.
> > > 
> > > Instead, I suggest we add some specific ones to test common scenarios such
> as
> > > the one that led to issue 611462 being
> > > filed.
> > 
> > The problem is that we didn't know the bug before it was filed (though I
could
> avoid it if I was more careful or discover it with other means) and adding
test
> coverage afterwards may be inadequate to prevent future errors like this.
> 
> The tests did provide coverage, we just didn't notice it properly. Adding a
ton
> of pixel expectations seems a high price to pay.
> 
> An interesting alternative: provide a simple script to test whether a patch
> caused a pixel difference in one of these tests. Then add
> debug output to them instructing developers to run the script if there were
any
> difference in repaints.
> 

This is interesting. Then I have a question: with this, can we say that all
pixel tests are unnecessary, but just let developers and try bots run the script
to check if a patch changed pixel results of any layout tests? I think it's
better to keep the consistency of testing tools for developers, try bots and
waterfall bots.

> > 
> > To avoid platform-specific expectations I can also convert the tests into
ref
> tests, but they will cost twice as pixel tests, and for now we don't support
> both text and ref expectations. However this is feasible. I've done this for
> paint/invalidation/spv2, with separated text and ref tests for text and ref
> expectations, respectively. I can pursue to add support of both text and ref
> expectations for layout tests to avoid the separated tests. Wdyt about this?

The reason that I converted repaint tests into ref tests for spv2 (under
paint/invalidation/spv2) was that text-based expectations may not catch all
errors in paint invalidation and painting, especially for spv2. As we are moving
more works planned for spv2 into spv1, I would like to merge the spv2-specific
tests back into their original tests so that spv1 can also have the coverage.
This will convert about 480 text-based repaint tests and 500 text-and-pixel
repaint tests into ref tests, and eliminate about 560 paint/invalidation/spv2
tests. What do you think about this plan?

[chrishtr, 2016-05-13 18:03:55]

On 2016/05/13 at 17:39:02, wangxianzhu wrote:
> On 2016/05/13 16:49:29, chrishtr wrote:
> > On 2016/05/13 at 16:31:39, wangxianzhu wrote:
> > > On 2016/05/13 15:43:06, chrishtr wrote:
> > > >
> >
https://codereview.chromium.org/1972273002/diff/50001/third_party/WebKit/Layo...
> > > > File third_party/WebKit/LayoutTests/fast/repaint/resources/repaint.js
> > (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/1972273002/diff/50001/third_party/WebKit/Layo...
> > > > third_party/WebKit/LayoutTests/fast/repaint/resources/repaint.js:6: if
> > > > (window.testRunner)
> > > > Pixel testing is expensive. It also leads to the requirement to
rebaseline
> > > > pixels for platform-specific expectations.
> > > > 
> > > > Instead, I suggest we add some specific ones to test common scenarios
such
> > as
> > > > the one that led to issue 611462 being
> > > > filed.
> > > 
> > > The problem is that we didn't know the bug before it was filed (though I
could
> > avoid it if I was more careful or discover it with other means) and adding
test
> > coverage afterwards may be inadequate to prevent future errors like this.
> > 
> > The tests did provide coverage, we just didn't notice it properly. Adding a
ton
> > of pixel expectations seems a high price to pay.
> > 
> > An interesting alternative: provide a simple script to test whether a patch
> > caused a pixel difference in one of these tests. Then add
> > debug output to them instructing developers to run the script if there were
any
> > difference in repaints.
> > 
> 
> This is interesting. Then I have a question: with this, can we say that all
pixel tests are unnecessary, but just let developers and try bots run the script
to check if a patch changed pixel results of any layout tests?

No, because some tests test only pixels, and may have platform differences.

>I think it's better to keep the consistency of testing tools for developers,
try bots and waterfall bots.

Yeah I see your point.

> 
> > > 
> > > To avoid platform-specific expectations I can also convert the tests into
ref
> > tests, but they will cost twice as pixel tests, and for now we don't support
> > both text and ref expectations. However this is feasible. I've done this for
> > paint/invalidation/spv2, with separated text and ref tests for text and ref
> > expectations, respectively. I can pursue to add support of both text and ref
> > expectations for layout tests to avoid the separated tests. Wdyt about this?
> 
> The reason that I converted repaint tests into ref tests for spv2 (under
paint/invalidation/spv2) was that text-based expectations may not catch all
errors in paint invalidation and painting, especially for spv2. As we are moving
more works planned for spv2 into spv1, I would like to merge the spv2-specific
tests back into their original tests so that spv1 can also have the coverage.
This will convert about 480 text-based repaint tests and 500 text-and-pixel
repaint tests into ref tests, and eliminate about 560 paint/invalidation/spv2
tests. What do you think about this plan?

Are you saying that SPv2 effectively already has this CL applied, and if that is
good for SPv2, why not apply it for SPv1 also?

[chrishtr, 2016-05-13 18:05:53]

On 2016/05/13 at 17:26:38, kbr wrote:
> On 2016/05/13 16:49:29, chrishtr wrote:
> > On 2016/05/13 at 16:31:39, wangxianzhu wrote:
> > > On 2016/05/13 15:43:06, chrishtr wrote:
> > > >
> >
https://codereview.chromium.org/1972273002/diff/50001/third_party/WebKit/Layo...
> > > > File third_party/WebKit/LayoutTests/fast/repaint/resources/repaint.js
> > (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/1972273002/diff/50001/third_party/WebKit/Layo...
> > > > third_party/WebKit/LayoutTests/fast/repaint/resources/repaint.js:6: if
> > > > (window.testRunner)
> > > > Pixel testing is expensive. It also leads to the requirement to
rebaseline
> > > > pixels for platform-specific expectations.
> > > > 
> > > > Instead, I suggest we add some specific ones to test common scenarios
such
> > as
> > > > the one that led to issue 611462 being
> > > > filed.
> > > 
> > > The problem is that we didn't know the bug before it was filed (though I
could
> > avoid it if I was more careful or discover it with other means) and adding
test
> > coverage afterwards may be inadequate to prevent future errors like this.
> > 
> > The tests did provide coverage, we just didn't notice it properly. Adding a
ton
> > of pixel expectations seems a high price to pay.
> > 
> > An interesting alternative: provide a simple script to test whether a patch
> > caused a pixel difference in one of these tests. Then add
> > debug output to them instructing developers to run the script if there were
any
> > difference in repaints.
> 
> It seems likely to me that this instruction will be ignored at some point.
Some sort of test that has pixel output by default is needed to make it obvious
to authors what went wrong. I would recommend either adding a couple of targeted
pixel tests for this failure (under-invalidation?), or converting just a few of
the repaint tests to pixel tests.

Right, I meant in addition to having some number of pixel tests.

> 
> > > To avoid platform-specific expectations I can also convert the tests into
ref
> > tests, but they will cost twice as pixel tests, and for now we don't support
> > both text and ref expectations. However this is feasible. I've done this for
> > paint/invalidation/spv2, with separated text and ref tests for text and ref
> > expectations, respectively. I can pursue to add support of both text and ref
> > expectations for layout tests to avoid the separated tests. Wdyt about this?
> 
> Maybe converting a couple of the repaint tests to reftests would be
sufficient?
> 
> I wouldn't constrain solutions based on machine capacity. The layout tests
should be sharded and swarmed, and more machines can be added.

It's not just machine capacity, but that is one factor that has been a real
constraint and remains so. For example, I spent about two months
begging for additional capacity for SPv2. Another example is that the
Blink->Chromium merge was delayed a long time due to capacity.
I don't think we can just ignore capacity here.

[Xianzhu, 2016-05-13 18:20:33]

Some data of the current layout tests:

- Number of repaint tests (not including paint/invalidation/spv2): 1006
  - text-only: 480
  - text-and-pixel: 526

- Time to run all repaint tests with the current configuration (tested on my
Linux machine):
  - 4 processes: 0m44s
  - 1 processes: 1m21s

- Time to run all repaint tests with the latest patch set:
  - 4 processes: 0m44s
  - 1 processes: 1m56s

It's interesting that with this patch the time with 4 processes isn't changed.
This is because we shard by directories, and svg/dynamic-updates contains the
biggest number of tests. I guess in a full layout test run, the added time for
the 480 originally text-only tests converted to pixel tests would just fill the
gaps of multi-process scheduling and won't increase the total time much.

Now I have the following plan:
1. Land the patch to layout test script to allow both text and ref expectations
of a test;
2. Merge back the ref tests under paint/invalidation/spv2 into their original
tests, and remove paint/invalidation/spv2;
3. For remaining text-only repaint tests, convert to ref tests or keep as-is;
4. May add virtual/spv2/fast/repaint (which is skipped by default and only
white-listed ones will run).

[Xianzhu, 2016-05-13 18:24:06]

> 
> It's interesting that with this patch the time with 4 processes isn't changed.
> This is because we shard by directories, and svg/dynamic-updates contains the
> biggest number of tests.

Note that the tests under svg/dynamic-updates are originally text-and-pixel
repaint tests.

[esprehn, 2016-05-16 05:01:22]

Note that your local workstation is many many times more powerful than the bots
for running tests. Locally the layout tests take a few minutes, they take 30m on
the bots.

[Xianzhu, 2016-05-16 21:09:10]

On 2016/05/16 05:01:22, esprehn wrote:
> Note that your local workstation is many many times more powerful than the
bots
> for running tests. Locally the layout tests take a few minutes, they take 30m
on
> the bots.

The main difference between local workstations and the bots might be the number
of cores, so I limited number of processes (4 and 1) when testing locally.

About real results, in the last run on linux_chromium_rel_ng with this patch,
webkit_tests took 26m15s. This is similar to other runs without this patch (in
the range between 24 to 28 minutes).

[Xianzhu, 2016-05-16 21:09:11]

On 2016/05/16 05:01:22, esprehn wrote:
> Note that your local workstation is many many times more powerful than the
bots
> for running tests. Locally the layout tests take a few minutes, they take 30m
on
> the bots.

The main difference between local workstations and the bots might be the number
of cores, so I limited number of processes (4 and 1) when testing locally.

About real results, in the last run on linux_chromium_rel_ng with this patch,
webkit_tests took 26m15s. This is similar to other runs without this patch (in
the range between 24 to 28 minutes).

[chrishtr, 2016-05-18 16:51:28]

@xianzhu: what's the status on this CL? Are you working on the alternatives you
suggested still?

[Xianzhu, 2016-05-18 16:59:19]

On 2016/05/18 16:51:28, chrishtr wrote:
> @xianzhu: what's the status on this CL? Are you working on the alternatives
you
> suggested still?

This is now blocked by https://codereview.chromium.org/1346673003/ which is step
1 in the plan in #14. Just pinged the CL. After the CL, this CL (maybe also
other CLs) will cover step 2 to 4.