Use testharness.js instead of js-test.js in LayoutTests/fast/canvas tests.

Use testharness.js instead of js-test.js in LayoutTests/fast/canvas tests.

This change replaces js-test.js assertions with testharness.js assertions.
The expectation files are removed as they are not needed anymore.
To simplify review and landing, each change corrects at most 10 tests.

BUG=639732
Review-Url: https://codereview.chromium.org/2681423002
Cr-Commit-Position: refs/heads/master@{#451776}
Committed: https://chromium.googlesource.com/chromium/src/+/bd743ec0094aa357859102b15e5fb339ea64fea0

[Justin Novosad, 2017-02-16 15:41:55]

junov@chromium.org changed reviewers:
+ junov@chromium.org

[Justin Novosad, 2017-02-16 15:41:56]

https://codereview.chromium.org/2681423002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-isPointInStroke-with-path.html
(right):

https://codereview.chromium.org/2681423002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/canvas/canvas-isPointInStroke-with-path.html:44:
assert_throws(null, function() {ctx.isPointInStroke(null, 70, 20);});
These asserts should be inside a test()

https://codereview.chromium.org/2681423002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/canvas/canvas-large-fills.html (right):

https://codereview.chromium.org/2681423002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/canvas/canvas-large-fills.html:49:
generate_tests(testLargeRect, testScenarios);
should not put generate_tests inside a test.

https://codereview.chromium.org/2681423002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/canvas/canvas-large-pattern.html
(right):

https://codereview.chromium.org/2681423002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/canvas/canvas-large-pattern.html:22:
//assert_array_equals(dstContext.getImageData(0, 0, 1, 1).data, [0, 255, 0,
255]);
Why are these commented-out? Without theses asserts, this test is not really
testing anything.

https://codereview.chromium.org/2681423002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/canvas/canvas-largedraws.html (right):

https://codereview.chromium.org/2681423002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/canvas/canvas-largedraws.html:13:
window.onload = function () {
onload is async. This code will end up being called after test has finished. 
Alternatively, you could call test() from within the onload handler.

[zakerinasab, 2017-02-16 18:12:40]

Comments addressed.

https://codereview.chromium.org/2681423002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-isPointInStroke-with-path.html
(right):

https://codereview.chromium.org/2681423002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/canvas/canvas-isPointInStroke-with-path.html:44:
assert_throws(null, function() {ctx.isPointInStroke(null, 70, 20);});
On 2017/02/16 15:41:55, Justin Novosad wrote:
> These asserts should be inside a test()

Done.

https://codereview.chromium.org/2681423002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/canvas/canvas-large-fills.html (right):

https://codereview.chromium.org/2681423002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/canvas/canvas-large-fills.html:49:
generate_tests(testLargeRect, testScenarios);
On 2017/02/16 15:41:56, Justin Novosad wrote:
> should not put generate_tests inside a test.

Acknowledged.

https://codereview.chromium.org/2681423002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/canvas/canvas-large-pattern.html
(right):

https://codereview.chromium.org/2681423002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/canvas/canvas-large-pattern.html:22:
//assert_array_equals(dstContext.getImageData(0, 0, 1, 1).data, [0, 255, 0,
255]);
On 2017/02/16 15:41:56, Justin Novosad wrote:
> Why are these commented-out? Without theses asserts, this test is not really
> testing anything.

I removed some comment tags. The thing is that this test is expected to fail to
read the pixel info (it's a crash test). The old -expected.txt file has the
FAILEDs already in place.
Any suggestions to put it in a better way?

https://codereview.chromium.org/2681423002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/canvas/canvas-largedraws.html (right):

https://codereview.chromium.org/2681423002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/canvas/canvas-largedraws.html:13:
window.onload = function () {
On 2017/02/16 15:41:56, Justin Novosad wrote:
> onload is async. This code will end up being called after test has finished. 
> Alternatively, you could call test() from within the onload handler.

Done.

[Justin Novosad, 2017-02-16 18:56:07]

> On 2017/02/16 15:41:56, Justin Novosad wrote:
> > Why are these commented-out? Without theses asserts, this test is not really
> > testing anything.
> 
> I removed some comment tags. The thing is that this test is expected to fail
to
> read the pixel info (it's a crash test). The old -expected.txt file has the
> FAILEDs already in place.
> Any suggestions to put it in a better way?

You can make it a failed expectation in TestExpectations. The harness
distinguishes between failures and crashes.

[zakerinasab, 2017-02-16 20:25:34]

On 2017/02/16 18:56:07, Justin Novosad wrote:
> > On 2017/02/16 15:41:56, Justin Novosad wrote:
> > > Why are these commented-out? Without theses asserts, this test is not
really
> > > testing anything.
> > 
> > I removed some comment tags. The thing is that this test is expected to fail
> to
> > read the pixel info (it's a crash test). The old -expected.txt file has the
> > FAILEDs already in place.
> > Any suggestions to put it in a better way?
> 
> You can make it a failed expectation in TestExpectations. The harness
> distinguishes between failures and crashes.

Done.

[Justin Novosad, 2017-02-17 16:17:34]

The CQ bit was checked by junov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 16:17:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Justin Novosad, 2017-02-17 16:19:52]

https://codereview.chromium.org/2681423002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2681423002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:767: crbug.com/281504
fast/canvas/canvas-large-pattern.html [ Failure ]
This crbug seems unrelated

[zakerinasab, 2017-02-17 16:33:17]

The CQ bit was unchecked by zakerinasab@chromium.org

[zakerinasab, 2017-02-17 16:37:54]

On 2017/02/17 16:19:52, Justin Novosad wrote:
>
https://codereview.chromium.org/2681423002/diff/40001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/2681423002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/TestExpectations:767: crbug.com/281504
> fast/canvas/canvas-large-pattern.html [ Failure ]
> This crbug seems unrelated

This bug is the one that originally introduced the FAILs in the expected test
results
(https://codereview.chromium.org/57643005/diff/1/LayoutTests/fast/canvas/canva...).
I'm not sure what is the correct thing to do here. The 32K limit on canvas size
from Skia is not a bug, so it doesn't seem right to file a new bug for that.
Adding some comments to the TestExpectations might be a solution, but I'm not
sure if it is a right thing to do too. WDYT?

[Justin Novosad, 2017-02-17 18:00:37]

On 2017/02/17 16:37:54, zakerinasab wrote:
> On 2017/02/17 16:19:52, Justin Novosad wrote:
> >
>
https://codereview.chromium.org/2681423002/diff/40001/third_party/WebKit/Layo...
> > File third_party/WebKit/LayoutTests/TestExpectations (right):
> > 
> >
>
https://codereview.chromium.org/2681423002/diff/40001/third_party/WebKit/Layo...
> > third_party/WebKit/LayoutTests/TestExpectations:767: crbug.com/281504
> > fast/canvas/canvas-large-pattern.html [ Failure ]
> > This crbug seems unrelated
> 
> This bug is the one that originally introduced the FAILs in the expected test
> results
>
(https://codereview.chromium.org/57643005/diff/1/LayoutTests/fast/canvas/canva...).
> I'm not sure what is the correct thing to do here. The 32K limit on canvas
size
> from Skia is not a bug, so it doesn't seem right to file a new bug for that.
> Adding some comments to the TestExpectations might be a solution, but I'm not
> sure if it is a right thing to do too.

[Justin Novosad, 2017-02-17 18:01:36]

Okay, you can leave it like this then, but you need to add the virtual/gpu and
virtual/display_list_2d_canvas variants to TestExpectations.

lgtm

[zakerinasab, 2017-02-21 15:06:56]

The CQ bit was checked by zakerinasab@chromium.org

[zakerinasab, 2017-02-21 15:06:57]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org
Link to the patchset: https://codereview.chromium.org/2681423002/#ps60001
(title: "Adding exceptions to TestExpectations")

[commit-bot: I haz the power, 2017-02-21 15:07:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 16:33:58]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487689616930860,
"parent_rev": "0dcca663b5138b8d79faf2702c3f9b5846938586", "commit_rev":
"bd743ec0094aa357859102b15e5fb339ea64fea0"}

[commit-bot: I haz the power, 2017-02-21 16:34:34]

Description was changed from

==========
Use testharness.js instead of js-test.js in LayoutTests/fast/canvas tests.

This change replaces js-test.js assertions with testharness.js assertions.
The expectation files are removed as they are not needed anymore.
To simplify review and landing, each change corrects at most 10 tests.

BUG=639732
==========

to

==========
Use testharness.js instead of js-test.js in LayoutTests/fast/canvas tests.

This change replaces js-test.js assertions with testharness.js assertions.
The expectation files are removed as they are not needed anymore.
To simplify review and landing, each change corrects at most 10 tests.

BUG=639732

Review-Url: https://codereview.chromium.org/2681423002
Cr-Commit-Position: refs/heads/master@{#451776}
Committed:
https://chromium.googlesource.com/chromium/src/+/bd743ec0094aa357859102b15e5f...
==========

[commit-bot: I haz the power, 2017-02-21 16:34:35]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/bd743ec0094aa357859102b15e5f...