1344744 - Update Telemetry unit tests to async function & await

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P4)

Description

[Georg Fritzsche [:gfritzsche]]

We have `async function` and `await` now:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function

We should be able to replace:
- `Task.async(function*(...))`, `Task.spawn(function*(...))` with a form of `async function ...`
- `yield` with `await` in the changed functions

The Telemetry test code is here:
https://dxr.mozilla.org/mozilla-central/search?q=path%3Atoolkit%2Fcomponents%2Ftelemetry%2Ftests%2F+Task.&redirect=false

The only case that doesn't migrate trivially is any `DeferredTask` usage, we could just leave those for now.

Comment 1

[Vedant Sareen [:fionn_mac]]

Attachment: Initial patch for Bug 1344744

Hello!

I found this bug to be a great learning opportunity, and so I've worked on it a bit.
I've attached an initial patch for this bug.

I was able to build successfully. I also executed |./mach xpcshell-test toolkit/components/telemetry|. The patch passed all tests.

Comment 2

[Deepjyoti Mondal]

Hi Vedant,

I am not sure whether the eslint test covers the test files too, but if it does then your patch will face eslint error.You need to use shorthand async functions as described here : https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions#Shorthand_async_methods

At first you may check eslint is causing any trouble at all. You can do so by this : 

./mach eslint toolkit/components/telemetry/

I faced the same problem once before so just though of letting you know about this :)

Comment 3

[Vedant Sareen [:fionn_mac]]

(In reply to Deepjyoti Mondal from comment #2)
> Hi Vedant,
> 
> I am not sure whether the eslint test covers the test files too, but if it
> does then your patch will face eslint error.You need to use shorthand async
> functions as described here :
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/
> Method_definitions#Shorthand_async_methods
> 
> At first you may check eslint is causing any trouble at all. You can do so
> by this : 
> 
> ./mach eslint toolkit/components/telemetry/
> 
> I faced the same problem once before so just though of letting you know
> about this :)

Oops, my patch did face an eslint error!
I had no idea that shorthand async functions are required here.

Thanks for the heads up! :)

Comment 4

[Vedant Sareen [:fionn_mac]]

Attachment: Initial patch for Bug 1344744

Updated patch with shorthand functions in appropriate locations.

I was able to build successfully. 
I also executed |./mach xpcshell-test toolkit/components/telemetry| and |./mach eslint toolkit/components/telemetry|. The patch passed all tests.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8845402 [details] [diff] [review]
Initial patch for Bug 1344744

Redirecting this one.

Comment 6

[Alessio Placitelli [:Dexter]]

Comment on attachment 8845402 [details] [diff] [review]
Initial patch for Bug 1344744

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

This looks good, thanks! Let's just make sure that we remove the Task.jsm import where/if it's no longer needed.

After that, make sure to run the tests again and check for linting errors!

::: toolkit/components/telemetry/tests/unit/TelemetryArchiveTesting.jsm
@@ +54,4 @@
>     * ]
>     * @returns a matching ping if found, or null
>     */
> +  async promiseFindPing(type, expected) {

Can we remove |Cu.import("resource://gre/modules/Task.jsm");| from the top of the file if it's not being used anymore?

::: toolkit/components/telemetry/tests/unit/head.js
@@ +96,4 @@
>      return this.promiseNextRequest().then(request => decodeRequestPayload(request));
>    },
>  
> +  async promiseNextRequests(count) {

Can we remove |Cu.import("resource://gre/modules/Task.jsm", this);| from the top of the file if it's not being used anymore?

::: toolkit/components/telemetry/tests/unit/test_PingAPI.js
@@ +37,4 @@
>   *                    type: <string>,
>   *                    size: <integer> }
>   */
> +var getArchivedPingsInfo = async function() {

Can we remove |Cu.import("resource://gre/modules/Task.jsm");| from the top of the file if it's not being used anymore?

::: toolkit/components/telemetry/tests/unit/test_SubsessionChaining.js
@@ +26,4 @@
>    return OS.Path.join(OS.Constants.Path.profileDir, "datareporting");
>  });
>  
> +var promiseValidateArchivedPings = async function(aExpectedReasons) {

Can we remove |Cu.import("resource://gre/modules/Task.jsm");| from the top of the file if it's not being used anymore?

::: toolkit/components/telemetry/tests/unit/test_TelemetrySend.js
@@ +52,4 @@
>    return id;
>  }
>  
> +var checkPingsSaved = async function(pingIds) {

Can we remove |Cu.import("resource://gre/modules/Task.jsm");| from the top of the file if it's not being used anymore?

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
@@ +50,4 @@
>   * @returns Promise
>   * @resolve an Array with the created pings ids.
>   */
> +var createSavedPings = async function(aPingInfos) {

Can we remove |Cu.import("resource://gre/modules/Task.jsm");| from the top of the file if it's not being used anymore?

Comment 7

[Vedant Sareen [:fionn_mac]]

Attachment: Rectified patch for Bug 1344744

Comment 8

[Vedant Sareen [:fionn_mac]]

I executed |./mach xpcshell-test toolkit/components/telemetry| and |./mach eslint toolkit/components/telemetry|. The rectified patch passed all tests.

Comment 9

[Alessio Placitelli [:Dexter]]

Comment on attachment 8845560 [details] [diff] [review]
Rectified patch for Bug 1344744

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

This looks good now, thanks Vedant.

Comment 10

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7797703595b2

Comment 11

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af8309c5eebaf0a90ed3bd1e87c05742513817a4
Bug 1344744 - Update Telemetry unit tests to async function & await. r=dexter

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/af8309c5eeba