Update Telemetry unit tests to async function & await

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Telemetry
P4
normal
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: gfritzsche, Assigned: fionn_mac, Mentored)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox54 wontfix, firefox55 fixed)

Details

(Whiteboard: [measurement:client] [lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 months ago
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.
(Reporter)

Updated

9 months ago
No longer depends on: 1344743
(Assignee)

Comment 1

9 months ago
Created attachment 8845128 [details] [diff] [review]
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.
Attachment #8845128 - Flags: review?(gfritzsche)

Comment 2

9 months ago
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 :)
(Assignee)

Comment 3

9 months ago
(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! :)
(Assignee)

Comment 4

9 months ago
Created attachment 8845402 [details] [diff] [review]
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.
Attachment #8845128 - Attachment is obsolete: true
Attachment #8845128 - Flags: review?(gfritzsche)
Attachment #8845402 - Flags: review?(gfritzsche)
(Reporter)

Updated

9 months ago
Assignee: nobody → vedant.sareen
(Reporter)

Comment 5

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

Redirecting this one.
Attachment #8845402 - Flags: review?(gfritzsche) → review?(alessio.placitelli)
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?
Attachment #8845402 - Flags: review?(alessio.placitelli) → feedback+
(Assignee)

Comment 7

9 months ago
Created attachment 8845560 [details] [diff] [review]
Rectified patch for Bug 1344744
Attachment #8845402 - Attachment is obsolete: true
Attachment #8845560 - Flags: review?(alessio.placitelli)
(Assignee)

Comment 8

9 months ago
I executed |./mach xpcshell-test toolkit/components/telemetry| and |./mach eslint toolkit/components/telemetry|. The rectified patch passed all tests.
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.
Attachment #8845560 - Flags: review?(alessio.placitelli) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7797703595b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/af8309c5eebaf0a90ed3bd1e87c05742513817a4
Bug 1344744 - Update Telemetry unit tests to async function & await. r=dexter

Comment 12

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/af8309c5eeba
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox54: affected → wontfix
You need to log in before you can comment on or make changes to this bug.