Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests for structuredClone #9218

Merged
merged 1 commit into from May 18, 2018

Conversation

@surma
Copy link
Contributor

surma commented Jan 28, 2018

Edit: This PR got side-tracked into refactoring the testing setup for algorithms that use structured clonging. The PR for adding tests for structuredClone (whatwg/html#3414) now lives in #11069.


This is in connection to whatwg/html#3414.

Do not merge. This still requires interest/agreement from other browser vendors.

@surma

This comment has been minimized.

Copy link
Contributor Author

surma commented Jan 28, 2018

I’m not sure to what extend I need to test the functionality of structuredClone considering that HTML’s structuredSerialize and structuredDeserialize are probably tested through other APIs already.

What’s the procedure here?

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Jan 28, 2018

Build PASSED

Started: 2018-03-21 15:18:58
Finished: 2018-03-21 15:27:28

Failing Jobs

  • build_css

View more information about this build on:

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jan 31, 2018

Ideally we find existing postMessage()/history state/etc. tests and generalize those to run on both methods. Note that even if methods share a primitive per the specification, you cannot trust implementations not to take shortcuts or do the wrong thing (they might also have more legitimate reasons to diverge for performance and such, and end up making a mistake).

@surma

This comment has been minimized.

Copy link
Contributor Author

surma commented Feb 1, 2018

Note to self: Look at tests below and create set of tests that can be imported into other API tests

  • html/browsers/history/the-history-interface
  • html/infrastructure/safe-passing-of-structured-data
  • workers/semantics/structured-clone

Corner-cases:

  • postMessage(Symbol(), "*", [new ArrayBuffer(2)]) throwing and not detaching the buffer
@surma

This comment has been minimized.

Copy link
Contributor Author

surma commented Mar 21, 2018

I’m back from vacation and continuing now to work on this.

The plan in the most recent commit is to create a battery of tests for the structured clone algorithm that can be reused by all APIs that use the algorithm.

Before I start porting a large number of tests and including this setup everywhere, I wanted to get a review if I am doing something stupid or re-inventing the wheel.

PTAL :)

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Mar 21, 2018

You want to remove the css/tools/w3ctestlib changes. It seems that at some point this should handle transferables as well?

I wonder a bit what all the infrastructure is needed for, but I'm guessing you have a plan of sorts.

@jeremyroman @bakulf @wanderview might have some thoughts on a good framework here as well.

};
runner = Object.assign({}, defaultRunner, runner);

await runner.setup();

This comment has been minimized.

Copy link
@jeremyroman

jeremyroman Mar 21, 2018

Contributor

If this setup function completes asynchronously, it is not guaranteed to happen before the page finishes loading. If promise_test doesn't run by then, I don't think the test will run correctly. (Not super experienced with WPT; let me know if there's some sneaky way this is safe.)

Instead, you could have each promise_test await the setup promise. (Redundantly awaiting the setup doesn't seem problematic.)

I'd suggest structuredCloneBatteryOfTests being a non-async function to reduce the risk of unexpectedly awaiting before test registration in general. The only interesting waiting that happens here seems to be between waiting for all tests to finish and tearing down.

This comment has been minimized.

Copy link
@surma

surma Mar 21, 2018

Author Contributor

TIL!

I like your suggestion, will implement it that way.

}, test.description);
}).catch(_ => {});
});
await Promise.all(allTests);

This comment has been minimized.

Copy link
@jeremyroman

jeremyroman Mar 21, 2018

Contributor

Since this appears to be the only interesting awaiting here, we could make the outer function non-async and just do:

Promise.all(allTests).then(() => runner.teardown());
@surma

This comment has been minimized.

Copy link
Contributor Author

surma commented Mar 21, 2018

I wonder a bit what all the infrastructure is needed for, but I'm guessing you have a plan of sorts.

@annevk I might very well be over-engineering. But considering I have to accommodate for workers to be set up and would like this setup to be future proof as well, it seems like a decent middle ground. Definitely open to other suggestions.

@surma

This comment has been minimized.

Copy link
Contributor Author

surma commented Mar 21, 2018

It seems that at some point this should handle transferables as well?

Yes, that will be added once I get there 😄 Wanna ensure this rig is acceptable to y’all first.

@surma

This comment has been minimized.

Copy link
Contributor Author

surma commented Apr 5, 2018

I pulled in all structured clone-specific bits of @zcorpan’s worker test-suite and made it reusable.
This should test pretty much every kind of data type.

I still need to write tests for the transferable bit, tho.

@surma surma force-pushed the surma-dump:structuredclone branch 2 times, most recently from 293ac7c to 7f943ba Apr 5, 2018

@zcorpan

This comment has been minimized.

Copy link
Contributor

zcorpan commented May 3, 2018

@surma the original tests do assertions in the worker and then pass the object back and do assertions again. I think this skips the middle part, right? Is that what we want?

It seems your new-dedicated.html test runs much better than the original, so maybe we can iterate here instead of #10820... :)

@zcorpan zcorpan self-requested a review May 3, 2018

@bakulf

This comment has been minimized.

Copy link
Contributor

bakulf commented May 3, 2018

Maybe it can help to share what we have in gecko. For gecko tests (mochitests) I wrote this test:
https://searchfox.org/mozilla-central/source/dom/base/test/test_postMessages.html
maybe some parts of this can be taken and converted to a WPTs.

Here the idea is to have separate tests for the different 'contexts' (for instance BroadcastChannel, window-to-window, workers, etc) but with a common core where clonable an transferable objects are tested.

@zcorpan

This comment has been minimized.

Copy link
Contributor

zcorpan commented May 4, 2018

Thanks @bakulf. I think that's the case with @surma's setup as well, right?

@surma can you remove the structuredClone test (for now) and add shared worker test, and remove the old tests? Then we can create a new PR for structuredClone (and window-to-window etc).

I see that you fixed createImageBitmap to work when it's returning a promise, excellent 👍

@surma

This comment has been minimized.

Copy link
Contributor Author

surma commented May 4, 2018

@surma can you remove the structuredClone test (for now) and add shared worker test,

Can do.

and remove the old tests?

What exactly do you mean?

Then we can create a new PR for structuredClone (and window-to-window etc).

My plan was to add a separate “battery of tests” that involve transferables. Should we punt on that for a separate PR as well?

(Also I’m currently busy with Google I/O, so won’t be able to actually do anything until after next week)

@zcorpan

This comment has been minimized.

Copy link
Contributor

zcorpan commented May 4, 2018

I mean removing workers/semantics/structured-clone/dedicated.html and workers/semantics/structured-clone/shared.html and their dependencies that are no longer used by your test.

I'm not in a rush, but would be nice to land something that fixes the old tests as they're currently disabled.

@zcorpan

This comment has been minimized.

Copy link
Contributor

zcorpan commented May 15, 2018

@surma friendly ping :)

@surma

This comment has been minimized.

Copy link
Contributor Author

surma commented May 17, 2018

Sorry for the delay. Now back from I/O, planning on working on this tomorrow :)

@surma

This comment has been minimized.

Copy link
Contributor Author

surma commented May 18, 2018

  • Removed mock structuredClone() test
  • Added test for SharedWorker
  • Added test infrastructure for structured cloning tests with transferables
  • Removed old DedicatedWorker and SharedWorker test

There’s only 2 very minimal tests for transferables, but I‘d prefer to finally land this and augment it in a separate PR (potentially with the help of others).

@zcorpan @annevk PTAL :)

@zcorpan

This comment has been minimized.

Copy link
Contributor

zcorpan commented May 18, 2018

Thanks!

@zcorpan

This comment has been minimized.

Copy link
Contributor

zcorpan commented May 18, 2018

Can you squash and rewrite the commit message to reflect the current state?

@surma surma force-pushed the surma-dump:structuredclone branch from fabd145 to e2bf5c8 May 18, 2018

@surma

This comment has been minimized.

Copy link
Contributor Author

surma commented May 18, 2018

@zcorpan done

@zcorpan zcorpan merged commit b8ef676 into web-platform-tests:master May 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@surma

This comment has been minimized.

Copy link
Contributor Author

surma commented May 18, 2018

🎉

@surma surma deleted the surma-dump:structuredclone branch May 18, 2018

hubot pushed a commit to WebKit/webkit that referenced this pull request May 21, 2018

cdumez@apple.com
File's structured serialization should serialize lastModified attribute
https://bugs.webkit.org/show_bug.cgi?id=185773

Reviewed by Youenn Fablet.

LayoutTests/imported/w3c:

Re-sync workers/semantics/structured-clone WPT tests after:
web-platform-tests/wpt#9218

* web-platform-tests/html/webappapis/structured-clone/structured-clone-battery-of-tests-harness.js: Added.
(runStructuredCloneBatteryOfTests):
* web-platform-tests/html/webappapis/structured-clone/structured-clone-battery-of-tests-with-transferables.js: Added.
(structuredCloneBatteryOfTests.push.async.f):
* web-platform-tests/html/webappapis/structured-clone/structured-clone-battery-of-tests.js: Renamed from LayoutTests/imported/w3c/web-platform-tests/workers/semantics/structured-clone/common.js.
(async.compare_Blob):
(get_canvas_1x1_transparent_black):
(get_canvas_1x1_non_transparent_non_black):
(compare_ImageBitmap):
(structuredCloneBatteryOfTests.push.async.f):
* web-platform-tests/html/webappapis/structured-clone/w3c-import.log: Copied from LayoutTests/imported/w3c/web-platform-tests/workers/semantics/structured-clone/w3c-import.log.
* web-platform-tests/workers/semantics/structured-clone/dedicated-expected.txt:
* web-platform-tests/workers/semantics/structured-clone/dedicated.html:
* web-platform-tests/workers/semantics/structured-clone/dedicated.js: Removed.
* web-platform-tests/workers/semantics/structured-clone/shared-expected.txt:
* web-platform-tests/workers/semantics/structured-clone/shared.html:
* web-platform-tests/workers/semantics/structured-clone/shared.js: Removed.
* web-platform-tests/workers/semantics/structured-clone/w3c-import.log:
* web-platform-tests/workers/semantics/structured-clone/worker-common.js: Removed.

Source/WebCore:

Update our implementation for the stuctured serialization of a File to include
its lastModified attribute, as per:
- https://w3c.github.io/FileAPI/#file-section

No new tests, rebaselined existing test.

* bindings/js/SerializedScriptValue.cpp:
(WebCore::CloneSerializer::write):
(WebCore::CloneDeserializer::readFile):
* fileapi/File.cpp:
(WebCore::File::File):
* fileapi/File.h:

LayoutTests:

Unskip structured serialization tests that no longer fail / time out.

* TestExpectations:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@232030 268f45cc-cd09-0410-ab3c-d52691b4dbfc

hubot pushed a commit to WebKit/webkit that referenced this pull request May 22, 2018

cdumez@apple.com
File's structured serialization should serialize lastModified attribute
https://bugs.webkit.org/show_bug.cgi?id=185773

Reviewed by Youenn Fablet.

LayoutTests/imported/w3c:

Re-sync workers/semantics/structured-clone WPT tests after:
web-platform-tests/wpt#9218

* web-platform-tests/html/webappapis/structured-clone/structured-clone-battery-of-tests-harness.js: Added.
(runStructuredCloneBatteryOfTests):
* web-platform-tests/html/webappapis/structured-clone/structured-clone-battery-of-tests-with-transferables.js: Added.
(structuredCloneBatteryOfTests.push.async.f):
* web-platform-tests/html/webappapis/structured-clone/structured-clone-battery-of-tests.js: Renamed from LayoutTests/imported/w3c/web-platform-tests/workers/semantics/structured-clone/common.js.
(async.compare_Blob):
(get_canvas_1x1_transparent_black):
(get_canvas_1x1_non_transparent_non_black):
(compare_ImageBitmap):
(structuredCloneBatteryOfTests.push.async.f):
* web-platform-tests/html/webappapis/structured-clone/w3c-import.log: Copied from LayoutTests/imported/w3c/web-platform-tests/workers/semantics/structured-clone/w3c-import.log.
* web-platform-tests/workers/semantics/structured-clone/dedicated-expected.txt:
* web-platform-tests/workers/semantics/structured-clone/dedicated.html:
* web-platform-tests/workers/semantics/structured-clone/dedicated.js: Removed.
* web-platform-tests/workers/semantics/structured-clone/shared-expected.txt:
* web-platform-tests/workers/semantics/structured-clone/shared.html:
* web-platform-tests/workers/semantics/structured-clone/shared.js: Removed.
* web-platform-tests/workers/semantics/structured-clone/w3c-import.log:
* web-platform-tests/workers/semantics/structured-clone/worker-common.js: Removed.

Source/WebCore:

Update our implementation for the stuctured serialization of a File to include
its lastModified attribute, as per:
- https://w3c.github.io/FileAPI/#file-section

No new tests, rebaselined existing test.

* bindings/js/SerializedScriptValue.cpp:
(WebCore::CloneSerializer::write):
(WebCore::CloneDeserializer::readFile):
* fileapi/File.cpp:
(WebCore::File::File):
* fileapi/File.h:

LayoutTests:

Unskip structured serialization tests that no longer fail / time out.

* TestExpectations:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@232043 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.