1243789 - Convert remaining mochitests to use tasks

Status: NEW

(Core :: DOM: Push Subscriptions, task, P5)

Description

[Lina Butler [:lina]]

We can flatten some of the promise chains in the Push mochitests with tasks. They're already used in `test_{data, permissions, register}.html`. These scripts should do the trick:

    <script type="text/javascript" src="/tests/SimpleTest/SpawnTask.js"></script>
    <script type="text/javascript" src="/tests/dom/push/test/test_utils.js"></script>

Comment 1

[Lina Butler [:lina]]

:kartikey0303 started on this in http://fpaste.org/318132/26130145.

In general, `promiseReturningFunc().then(result => { /* ... */ })` becomes `var result = yield promiseReturningFunc()`. Also, the second argument to `.then()` handles rejected promises (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then).

Rejections are similar to exceptions in synchronous code; in fact, `add_task` automatically translates them for you. Here's how `setupPushNotification` would look:

    add_task(function* setupPushNotification() {
      try {
        var pushSubscription = yield registration.pushManager.subscribe();
        ok(true, "successful registered for push notification");
      } catch (error) {
        ok(false, "could not register for push notification");
      }
    });

Although the `try...catch` is redundant in this case, since an exception will fail the test. So we can simplify it to:

    add_task(function* setupPushNotification() {
      var pushSubscription = yield registration.pushManager.subscribe();
      ok(true, "successful registered for push notification");
    });

The second thing to keep in mind is that the tasks will be executed in order. For `test_unregister.html`, the order (from `runTest`) is: start, setupPushNotification, unregisterPushNotification, unregisterAgain, unregisterSW.

Comment 2

[Lina Butler [:lina]]

Answering a follow-up question from IRC...

`runTest` shows you the order in which the tests will execute. For example: https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/dom/push/test/test_unregister.html#72-76

Comment 3

[Lina Butler [:lina]]

How's it going, :kartikey0303? Can I help with anything?

Comment 4

[Kartikey Agrawal[:kartikey0303]]

Really sorry :kitcambridge. I have my exams going on. I have converted two files to the new format. Working on the rest. Very sorry for the delay.

Comment 5

[Lina Butler [:lina]]

No worries at all! Please focus on your exams, and don't fret about this bug. I just wanted to make sure you weren't stuck. :-)

Comment 6

[Kartikey Agrawal[:kartikey0303]]

Attachment: bug-1243789_unregister.diff

I tried to work on one of the file. If it is correct then i will do the other files. Very sorry for the delay. This bug was a little tough for me to understand (this being my one of the initial bugs). Please also tell me if something was wrong and what should i read to correct it.

Comment 7

[Lina Butler [:lina]]

Comment on attachment 8723521 [details] [diff] [review]
bug-1243789_unregister.diff

Review of attachment 8723521 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the review delay on my end. This is looking good, especially considering you haven't worked with promises or generators before! I left a few comments, mostly nits and small fixes.

::: dom/push/test/test_unregister.html
@@ +25,5 @@
>  
>  <script class="testbody" type="text/javascript">
>  
> +var registration;
> +add_task(function* start() {

Please add these lines before registering the service worker, to match `test_data.html`:

  yield setupPrefs();
  yield setPushPermission(true);

@@ +32,3 @@
>  
> +add_task(function* setupPushNotification(){
> +    var pushSubscription=yield registration.pushManager.subscribe();

A couple of style nits:

* Please add a space before and after `=`, here and in `unregisterPushNotification`.
* Indentation is two spaces.

@@ +39,5 @@
> +    try{
> +      var result= yield registration.unsubscribe();
> +      ok(result, "unsubscribe() on existing subscription should return true.");
> +    }
> +    catch(error){

You can remove the `try...catch` block. If `registration.unsubscribe()` throws, it'll fail the test, anyway.

@@ +49,5 @@
> +    try{
> +      var result= yield registration.unsubscribe();
> +      ok(!result, "unsubscribe() on previously unsubscribed subscription should return false.");
> +    } catch(error){
> +      ok(false, "unsubscribe() should never fail.");

Same as above.

@@ +62,2 @@
>  
>    SpecialPowers.pushPrefEnv({"set": [

If you add `setupPrefs()` and `setupPrefs()` to the `start` function, you can remove the calls to `pushPrefEnv` and `addPermission`.

Also, please remove `SimpleTest.waitForExplicitFinish()`. The task helper does this automatically.

Comment 8

[yoogottamk]

Hello! I am a newcomer. I want to work on this bug. Is this a good issue?

Comment 9

[Lina Butler [:lina]]

Sure! 😊 These are the tests in dom/push/test that we need to convert:

• test_multiple_register.html
• test_multiple_register_during_service_activation.html
• test_try_registering_offline_disabled.html

First, you'll need to build Firefox, using an artifact build. Then, you'll want to add the two scripts I mentioned in comment 0 to those HTML files, and change the tests to use add_task. If you get stuck, check out comment 1 or attachment 8723521 [details] [diff] [review], or feel free to ask me!

Comment 10

[yoogottamk]

Attachment: patch.diff

Hello!

I tried to work on one of the files in the list. Can you please check if it correct? Thanks a lot!

Comment 11

[Lina Butler [:lina]]

Comment on attachment 9050176 [details] [diff] [review]
patch.diff

Review of attachment 9050176 [details] [diff] [review]:
-----------------------------------------------------------------

This is a great start! Thanks for taking this on. 😊

::: dom/push/test/test_multiple_register.html
@@ +30,5 @@
>    //  console.log(str + "\n");
>    }
>  
> +  var registration;
> +  add_task(function* start() {

Firefox has supported async functions (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function) for a while, so let's make this `async function start`...

@@ +31,5 @@
>    }
>  
> +  var registration;
> +  add_task(function* start() {
> +    yield setupPrefsAndMockSocket(new MockWebSocket());

And change `yield` to `await`, here and everywhere else.

@@ +41,4 @@
>  
> +  add_task(function* getEndpointExpectNull() {
> +    var pushSubscription = yield registration.pushManager.getSubscription();
> +    ok(pushSubscription == null, "getEndpoint should return null when app not subscribed.");

Instead of using `ok(a == b, "message")`, let's use `is(a, b, "message")`, so that we get more descriptive errors.

Comment 12

[yoogottamk]

Attachment: patch.diff

Hello!

Thanks for pointing out my mistakes. I have learnt from them.

• Converted all function* f() to async function f
• Changed all yields to awaits
• Converted ok(a == b, "some message if a == b") to is(a, b, "message if a != b") [This is how it was being used in test_data.html]

Can you please review them? Thanks a lot!!

Comment 13

[yoogottamk]

Attachment: patch.diff [fixed a small issue with previous patch]

Comment 14

[Lina Butler [:lina]]

Comment on attachment 9050269 [details] [diff] [review]
patch.diff [fixed a small issue with previous patch]

Review of attachment 9050269 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect!

If you didn't know this already, you can use `mach mochitest dom/push/test/test_multiple_register.html` to run the test, or `mach mochitest dom/push/test` to run all the tests. :-)

::: dom/push/test/test_multiple_register.html
@@ +13,4 @@
>    <script type="text/javascript" src="/tests/dom/push/test/test_utils.js"></script>
>    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
>    <meta http-equiv="Content-type" content="text/html;charset=UTF-8">
> +  <script type="text/javascript" src="/tests/SimpleTest/SpawnTask.js"></script>

Whoops, it looks like `SpawnTask.js` is now called `AddTask.js`.

Comment 15

[yoogottamk]

Hello! Thanks for pointing out that mistake and teaching me how to run tests [I did not know that]!

I have a few questions. Can you please answer them? Thanks!

1. I have finished the work on the file test_multiple_register.html. I am doing the same for test_multiple_register_during_service_activation.html now. I found that a function setupMultipleSubscriptions does the same job as setupSecondEndpoint and getEndpoint do in test_multiple_register.html, [i.e., subscribe twice and check if both their endpoints are same], but here it is done in a different manner. Would adding two tasks and checking like I did before be equivalent to what is being done here?

2. I looked at the file test_try_registering_offline_disabled.html. It is using the same functions multiple times and it runs 6 tests using mostly the same set of functions. Here, I was thinking of defining the function and calling it inside add_task for each test. Is this approach valid?

3. How should I submit patches for multiple files? Should I attach a patch for each file or one single patch with changes for all the files?

Thank you again!

Comment 16

[yoogottamk]

Attachment: patch.diff

Hello!

I changed the name of the script in the first file and finished the work in the second file. [Attached here as a single patch]

I will work on the third one soon.

Comment 17

[Lina Butler [:lina]]

Comment on attachment 9051022 [details] [diff] [review]
patch.diff

Review of attachment 9051022 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work!

`test_multiple_register_during_service_activation.html` does something a bit different...the important part is that it restarts the push service, then calls `subscribe` concurrently (`await subscribe(swr); await subscribe(swr);` _would_ do the same thing as `setupSecondEndpoint` and `getEndpoint`, in that it waits for the firt `subscribe` to finish, but `await Promise.all([subscribe(swr), subscribe(swr)])` doesn't wait for the first `subscribe` call to finish before the second).

This makes sure that, when the service finishes restarting, we didn't try to create two different subscriptions for the same worker, even if we didn't wait for `subscribe` to resolve.

::: dom/push/test/test_multiple_register_during_service_activation.html
@@ +48,3 @@
>  
> +  var pushSubscription;
> +  add_task(async function getEndpoint() {

This isn't quite right. For this test, we want to copy `setupMultipleSubscriptions`. Something like this; you'd flatten the `then` handlers into `await`s, just like you did in the first file:

    let a = await Promise.all([...]);
    ok(a[0].endpoint == a[1].endpoint, ...);
    pushSubscription = a[0];
    await promiseTeardown;

@@ +51,5 @@
> +    pushSubscription = await registration.pushManager.subscribe();
> +    ok(true, "successfully registered for push notification");
> +  });
> +
> +  add_task(async function getSecondEndpoint() {

...And then you can just unsubscribe in this function (or in `unregister`), no need to check the endpoint again.

Comment 18

[Lina Butler [:lina]]

Hi!

(In reply to yoogottamk from comment #15)

2. I looked at the file test_try_registering_offline_disabled.html. It is using the same functions multiple times and it runs 6 tests using mostly the same set of functions. Here, I was thinking of defining the function and calling it inside add_task for each test. Is this approach valid?

That's perfect, thanks!

3. How should I submit patches for multiple files? Should I attach a patch for each file or one single patch with changes for all the files?

A single patch with changes for all the files, please.

Thanks for working on this!

Comment 19

[yoogottamk]

Attachment: patch.diff

Hello! I finished working on the third file as well!

It was a great learning experience!

(Although I think I have spammed the all runTestxs with unnecessary awaits
for eg

• changePushServerConnectionEnabled should / does not require await
• I added await in front of every function that should do the job without await as well
  )

Please tell me if I should remove them.

Thanks a lot!!

Comment 20

[yoogottamk]

Hello!

Is there anything else I need to do?

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.