search

web-platform-tests / wpt

Test suites for Web platform specs — including WHATWG, W3C, and others
https://web-platform-tests.org/
Other
4.88k stars 3.05k forks source link

Many offscreen-canvas tests fail because OffscreenCanvasRenderingContext2D.drawImage is called with a blob #20867

Open Cwiiis opened 4 years ago

Cwiiis commented 4 years ago

blob is not a valid CanvasImageSource, but many of the offscreen-canvas tests are written as if it is. These tests all fail. Will submit a PR fixing them by using ImageBitmap (many of these are worker tests, so Image isn't always an option and I assume why Image wasn't used in the first place).

guest271314 commented 4 years ago

Can you cite the tests that you are referring to?

Cwiiis commented 4 years ago

Can you cite the tests that you are referring to?

There are too many to quote, easier to just look at the changed file list in the pull request.

guest271314 commented 4 years ago

Might as well remove the redundant creation of a second OffscreenCanvas 

var offscreenCanvas = new OffscreenCanvas(100, 50);
var ctx = offscreenCanvas.getContext('2d');
Cwiiis commented 4 years ago

Might as well remove the redundant creation of a second OffscreenCanvas

var offscreenCanvas = new OffscreenCanvas(100, 50);
var ctx = offscreenCanvas.getContext('2d');

Is this referring to a particular file/section? I don't see what's wrong with that code in isolation.

guest271314 commented 4 years ago

@Cwiiis See https://github.com/web-platform-tests/wpt/pull/20776. @jdm might have deleted their comments from the PR. There is "yaml" and a "template" file which inserts that code into tests. See https://github.com/Cwiiis/wpt/blob/2aeaa11861b12de3c12de4ddc3076ddb1c10bc7a/offscreen-canvas/compositing/2d.composite.canvas.copy.html#L20 - line 21. Are two OffscreenCanvas instances necessary to perform the test?

guest271314 commented 4 years ago

You can observe the same occurs at https://github.com/web-platform-tests/wpt/issues/19799

Cwiiis commented 4 years ago

@Cwiiis See #20776. @jdm might have deleted their comments from the PR. There is "yaml" and a "template" file which inserts that code into tests. See https://github.com/Cwiiis/wpt/blob/2aeaa11861b12de3c12de4ddc3076ddb1c10bc7a/offscreen-canvas/compositing/2d.composite.canvas.copy.html#L20 - line 21. Are two OffscreenCanvas instances necessary to perform the test?

It is required, yes (assuming that's what the author wanted to test). The second canvas is used as an image source for the first and then the contents of the first are tested. I don't know that the image is really required here, as opposed to just using fillRect on the second canvas, but it doesn't hurt.

Cwiiis commented 4 years ago

This is really besides the point though, if you have issues with the tests themselves, best to file separate issues - this issue is about the generated tests doing something invalid, not about the tests themselves. I'd rather not conflate multiple issues.

guest271314 commented 4 years ago

this issue is about the generated tests doing something invalid, not about the tests themselves.

What is the practical difference?

If we are going to fix the tests, might as well fix all of the issues in the tests.

It is your issue and PR, you can do as you please.

@jdm Where did you post about the "yaml" and "template" code which inserts 


var offscreenCanvas = new OffscreenCanvas(100, 50);
var ctx = offscreenCanvas.getContext('2d');

into the OffscreenCanvas tests?

jdm commented 4 years ago

That was in https://github.com/web-platform-tests/wpt/pull/19818#issuecomment-566848962.

Cwiiis commented 4 years ago

this issue is about the generated tests doing something invalid, not about the tests themselves.

What is the practical difference?

One is the tests aren't actually running, the second is there are issues with what the test is trying to do. I'm fixing the former. I actually don't think there's anything wrong with the tests here either, at least not the one you've pointed to.

If we are going to fix the tests, might as well fix all of the issues in the tests.

I vehemently disagree with this. Better to make many small, easy-to-vet changes than to make a bunch of possibly-unrelated changes together. Though this diff is huge, the actual change is tiny (wrap every image response in a createImageBitmap call so that the parameters are valid for the tests).

If every time you touched something you had to fix every small issue with it, it would be very hard to get anything done and makes reviewers jobs much harder too.

guest271314 commented 4 years ago

I vehemently disagree with this. Better to make many small, easy-to-vet changes than to make a bunch of possibly-unrelated changes together. Though this diff is huge, the actual change is tiny (wrap every image response in a createImageBitmap call so that the parameters are valid for the tests).

If every time you touched something you had to fix every small issue with it, it would be very hard to get anything done and makes reviewers jobs much harder too.

Well, yes, you can modify the existing code to use createImageBitmap() where required and ignore the remainder of the test.

It would be helpful to actually determine what the expected result of the test is.

For example, what exactly is the expected result of this test?

The title of the test is "OffscreenCanvas test: 2d.drawImage.zerosource". The desccription of the test is "drawImage with zero-sized source rectangle throws INDEX_SIZE_ERR".

What is meant by "zero-sized source rectangle"? And is that what the current version of the code actually does?