third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html - Issue 2401013002: Make MediaSession per frame as an attribute of Navigator

Unified Diff: third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html

Issue 2401013002: Make MediaSession per frame as an attribute of Navigator (Closed)

Patch Set: fixed tests Created 4 years, 2 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« third_party/WebKit/LayoutTests/media/mediasession/mediasession-constructor-hidden.html ('K') | « third_party/WebKit/LayoutTests/media/mediasession/mediasession-constructor-hidden.html ('k') | third_party/WebKit/LayoutTests/media/mediasession/mediasessionmetadata.html » ('j') | third_party/WebKit/LayoutTests/media/mediasession/mediasessionmetadata.html » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html

diff --git a/third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html b/third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html

new file mode 100644

index 0000000000000000000000000000000000000000..db1409b09506bb94de14727d212170c9b47aba55

--- /dev/null

+++ b/third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html

@@ -0,0 +1,120 @@

+<!DOCTYPE html>

+<title>MediaSession Integration Test</title>

+<script src="../../resources/testharness.js"></script>

+<script src="../../resources/testharnessreport.js"></script>

+<script src="../../resources/mojo-helpers.js"></script>

+<script src="resources/mediasessionservice-mock.js"></script>

+<script src="resources/utils.js"></script>

+<script>

+var mock;

+// Each entry contains a test object and a test function.

+var pendingTests = [];

+// All the tests steps are wrapped into a function, and each test should invoke

+// runPendingTests() on finish.

+function runPendingTests() {

+ if (pendingTests.length < 1) {

+ return;

+ }

+ var tmp = pendingTests.pop();

+ var testObj = tmp[0];

+ var testFunc = tmp[1];

+ testFunc(testObj);

+promise_test(t => {

+ return mediaSessionServiceMock.then(m => {

whywhat 2016/10/10 20:45:09 can't we just setup the mock up for each test like

Zhiqiang Zhang (Slow) 2016/10/10 21:21:18 Our test here is slightly different. It's because

whywhat 2016/10/11 14:58:33 the budget service tests seem to just create a new

Zhiqiang Zhang (Slow) 2016/10/11 16:59:05 I think in our case, all these tests are used for

I think in our case, all these tests are used for testing if the Metadata is correctly passed to content/browser. These tests are isomorphic so I would prefer not to split them into separate files. Ideally it's possible write more similar tests in the future, and then we need to write new files, and each file would have the same boilerplate code. If we change a bit in the API in the future, all these files need to be modified. I think the budget tests was split into multiple files since it's not likely to have more similar tests. So I think it's fine since we have a good degree of test isolation, since the tests execute one after another and there is no chance they have overlapping lifetimes.

whywhat 2016/10/11 18:59:45 First, I don't think there's that much boilerplate

On 2016/10/11 at 16:59:05, Zhiqiang Zhang wrote: > On 2016/10/11 at 14:58:33, whywhat wrote: > > On 2016/10/10 at 21:21:18, Zhiqiang Zhang wrote: > > > On 2016/10/10 20:45:09, whywhat wrote: > > > > can't we just setup the mock up for each test like in > > > > budget-service-mock.js-using tests? > > > > the setup to add order to tests seems very unusual :/ > > > > There's also setup(function()) in testharness.js I believe, maybe it could help. > > > > > > Our test here is slightly different. It's because we are sending the metadata through mojo and the browser end receives the metadata asynchronously. I looked at what the budget service test do and seems doesn't fit our needs. Also, setup() in testharness.js is synchronous :( > > > > > > I think the tests in this file are very similar so I prefer not to split them into multiple files. The reason why I run them one after another is to avoid any chances of race. What I'm thinking is that multiple async_test could be executed in parallel. There's an issue on testharness.js discussing executing async_tests in sequence. https://github.com/w3c/testharness.js/issues/96 > > > I'll try do some cleanup and see if it feels better. > > > > the budget service tests seem to just create a new mock for every promise test. why can't we do that? > > async + mojo is common, so I don't see how we're different here. > > I think in our case, all these tests are used for testing if the Metadata is correctly passed to content/browser. These tests are isomorphic so I would prefer not to split them into separate files. Ideally it's possible write more similar tests in the future, and then we need to write new files, and each file would have the same boilerplate code. If we change a bit in the API in the future, all these files need to be modified. I think the budget tests was split into multiple files since it's not likely to have more similar tests. > > So I think it's fine since we have a good degree of test isolation, since the tests execute one after another and there is no chance they have overlapping lifetimes.

First, I don't think there's that much boilerplate. At least the boiler plate will be familiar to the readers compared to inventing a concept of running tests in sequence with test harness. Second, I should've been more specific about the budget tests: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... has two tests that create a new mock each. There're a couple of other test files using the same technique (get-budget.html and get-cost.html). So you should be able to just do something like this... async_test(function(test) { MediaSessionServiceMock.then(t.step_func(function() { var metadata = ...; mock.setMetadataStub = t.step_func_done(m) { assert_metadata_equals(metadata, mojoMetadataToJS(m)); }; navigator.mediaSession.metadata = metadata; }), test.unreached_func("Setting up MediaSession service mock failed!")); }, "test that MediaMetadata is correctly propagated"); ...for each test.

Zhiqiang Zhang (Slow) 2016/10/11 19:39:05 OK. I didn't found any other tests running async_t

On 2016/10/11 18:59:45, whywhat wrote: > On 2016/10/11 at 16:59:05, Zhiqiang Zhang wrote: > > On 2016/10/11 at 14:58:33, whywhat wrote: > > > On 2016/10/10 at 21:21:18, Zhiqiang Zhang wrote: > > > > On 2016/10/10 20:45:09, whywhat wrote: > > > > > can't we just setup the mock up for each test like in > > > > > budget-service-mock.js-using tests? > > > > > the setup to add order to tests seems very unusual :/ > > > > > There's also setup(function()) in testharness.js I believe, maybe it > could help. > > > > > > > > Our test here is slightly different. It's because we are sending the > metadata through mojo and the browser end receives the metadata asynchronously. > I looked at what the budget service test do and seems doesn't fit our needs. > Also, setup() in testharness.js is synchronous :( > > > > > > > > I think the tests in this file are very similar so I prefer not to split > them into multiple files. The reason why I run them one after another is to > avoid any chances of race. What I'm thinking is that multiple async_test could > be executed in parallel. There's an issue on testharness.js discussing executing > async_tests in sequence. https://github.com/w3c/testharness.js/issues/96 > > > > I'll try do some cleanup and see if it feels better. > > > > > > the budget service tests seem to just create a new mock for every promise > test. why can't we do that? > > > async + mojo is common, so I don't see how we're different here. > > > > I think in our case, all these tests are used for testing if the Metadata is > correctly passed to content/browser. These tests are isomorphic so I would > prefer not to split them into separate files. Ideally it's possible write more > similar tests in the future, and then we need to write new files, and each file > would have the same boilerplate code. If we change a bit in the API in the > future, all these files need to be modified. I think the budget tests was split > into multiple files since it's not likely to have more similar tests. > > > > So I think it's fine since we have a good degree of test isolation, since the > tests execute one after another and there is no chance they have overlapping > lifetimes. > > First, I don't think there's that much boilerplate. At least the boiler plate > will be familiar to the readers compared to inventing a concept of running tests > in sequence with test harness. > > Second, I should've been more specific about the budget tests: > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... > has two tests that create a new mock each. There're a couple of other test files > using the same technique (get-budget.html and get-cost.html). > > So you should be able to just do something like this... > > async_test(function(test) { > MediaSessionServiceMock.then(t.step_func(function() { > var metadata = ...; > mock.setMetadataStub = t.step_func_done(m) { > assert_metadata_equals(metadata, mojoMetadataToJS(m)); > }; > navigator.mediaSession.metadata = metadata; > }), > test.unreached_func("Setting up MediaSession service mock failed!")); > }, "test that MediaMetadata is correctly propagated"); > > ...for each test.

OK. I didn't found any other tests running async_tests in sequence. I'm splitting the tests now, and moved them to a separate mojo/ directory.

+ mock = m;

+ runPendingTests();

+ });

+}, "setting up mock MediaSession service");

+function testMetadataPropagated(t) {

+ var metadata = new MediaMetadata({

+ title: "title1",

+ artist: "artist1",

+ album: "album1",

+ artwork: [

+ { src: "http://foo.com/bar.png", type: "image/png", sizes: "128x128" }

+ ]});

+ mock.setMetadataStub = function(mojoMetadata) {

whywhat 2016/10/11 18:59:45 I believe you can replace it with step_func(functi

Zhiqiang Zhang (Slow) 2016/10/11 19:39:05 Done.

+ t.step(() => {

+ assert_metadata_equals(metadata, mojoMetadataToJS(mojoMetadata));

mlamouri (slow - plz ping) 2016/10/10 20:30:34 I don't think you should call `mojoMetadataToJS` i

Zhiqiang Zhang (Slow) 2016/10/11 10:56:03 Done.

+ });

+ t.done();

+ runPendingTests();

+ };

+ window.navigator.mediaSession.metadata = metadata;

+async_test(t => {

+ pendingTests.push([t, testMetadataPropagated]);

+}, "test that MediaMetadata is correctly propagated");

+function testMetadataPropagatedTwice(t) {

+ var dontCareMetadata = new MediaMetadata({});

+ mock.setMetadataStub = function() {

+ var metadata = new MediaMetadata({

+ title: "title2",

+ artist: "artist2",

+ album: "album2",

+ artwork: [

+ { src: "http://foo.com/bar.jpg", type: "image/jpeg", sizes: "256x256"}

+ ]});

+ mock.setMetadataStub = function(mojoMetadata) {

+ t.step(() => {

+ assert_metadata_equals(metadata, mojoMetadataToJS(mojoMetadata));

+ });

+ t.done();

+ runPendingTests();

+ }

+ window.navigator.mediaSession.metadata = metadata;

+ }

+ window.navigator.mediaSession.metadata = dontCareMetadata;

+async_test(t => {

+ pendingTests.push([t, testMetadataPropagatedTwice]);

+}, "test that MediaMetadata is correctly propagated twice");

+function testSetNullMetadata(t) {

+ mock.setMetadataStub = function(mojoMetadata) {

+ t.step(() => {

+ assert_false(!!mojoMetadata);

+ });

+ t.done();

+ runPendingTests();

+ };

+ window.navigator.mediaSession.metadata = null;

+async_test(t => {

+ pendingTests.push([t, testSetNullMetadata]);

+}, "test that setting null MediaMetadata");

+function testFileImageRemoved(t) {

+ var metadata = new MediaMetadata({

+ artwork: [

+ { src: "file:///foo/bar.jpg", type: "image/jpeg"}

+ ]});

+ var expectedMetadata = new MediaMetadata({});

+ mock.setMetadataStub = function(mojoMetadata) {

+ t.step(() => {

+ assert_metadata_equals(expectedMetadata, mojoMetadataToJS(mojoMetadata));

+ });

+ t.done();

+ runPendingTests();

+ }

+ window.navigator.mediaSession.metadata = metadata;

+async_test(t => {

+ pendingTests.push([t, testFileImageRemoved]);

+}, "test that file MediaImage is removed");

+</script>