1230213 - test_TelemetryLog yields from a non generator function

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Alessio Placitelli [:Dexter]]

In test_TelemetryLog.js [0] we're using |yield| within a non-generator function.

Since bug 982852 landed, we can solve this problem by:

- Changing |function run_test()| to |add_task(function* ()|.
- Add |do_get_profile();| before |yield TelemetrySession.setup();|.
- Add the missing ")" at the end of the function.

To make sure that everything works as expected:

./mach xpcshell-test toolkit/components/telemetry/tests/unit/test_TelemetryLog.js
./mach xpcshell-test toolkit/components/telemetry/

[0] - https://dxr.mozilla.org/mozilla-central/rev/eec9a0bfd929a68237f0ef799a9d8f28c4749296/toolkit/components/telemetry/tests/unit/test_TelemetryLog.js#27

Comment 1

[Salah]

Hi, I would like to work on this bug. Could you give me some starter instructions on how to begin. Thanks

Comment 2

[Alessio Placitelli [:Dexter]]

Hello Salah, and welcome!

I've assigned you this bug. The very first post here contains some instructions on how to fix the reported issue. Please feel free to contact me if anything is unclear, either here or over IRC!

Comment 3

[Salah]

Attachment: Bug fix attempt

Ive made the proposed changes. Not sure exactly where to add the final ).

Comment 4

[Alessio Placitelli [:Dexter]]

Comment on attachment 8696364 [details] [diff] [review]
Bug fix attempt

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

This looks on the right path, thanks.

The patch is missing the commit message, you can use "Bug 1230213: test_TelemetryLog yields from a non generator function. r?dexter" for it.

As for the missing ")", you need to add it to the end of the function wrapped by add_task, basically after the last "}" in the file, here: https://dxr.mozilla.org/mozilla-central/rev/cc9c6cd756cb744596ba039dcc5ad3065a7cc3ea/toolkit/components/telemetry/tests/unit/test_TelemetryLog.js#48

Please make sure that the patch successfully passes the tests from the bug description.

Comment 5

[Salah]

Attachment: patch1230213[1].patch

Tests passed and changes made

Comment 6

[Alessio Placitelli [:Dexter]]

Thanks for the update. The change looks good, but would you mind putting all the changes in a single file and flagging me for review on that one?

We're nearly there!

Comment 7

[Salah]

Attachment: patch1230213[Final].patch

Final Changes

Comment 8

[Alessio Placitelli [:Dexter]]

Comment on attachment 8696649 [details] [diff] [review]
patch1230213[Final].patch

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

This looks ok, thanks.
I'm pushing it to try for you: if everything is ok, we will land this!

Comment 9

[Alessio Placitelli [:Dexter]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c93aabab57dc

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8696649 [details] [diff] [review]
patch1230213[Final].patch

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

::: toolkit/components/telemetry/tests/unit/test_TelemetryLog.js
@@ +43,5 @@
>   check_event(log[2], TEST_PREFIX + "3", undefined);
>   do_check_true(log[0][1] <= log[1][1]);
>   do_check_true(log[1][1] <= log[2][1]);
>   yield TelemetrySession.shutdown();
> +})

A nit here: missing semicolon at the end of this line.

Comment 11

[Salah]

Attachment: patch1230213.patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8696843 [details] [diff] [review]
patch1230213.patch

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

::: toolkit/components/telemetry/tests/unit/test_TelemetryLog.js
@@ +28,3 @@
>  {
> +  do_get_profile();
> +   yield TelemetrySession.setup();

This line is now indented three spaces, an oversight?
It should be indented by two spaces.
Looks good otherwise :)

Comment 13

[Salah]

(In reply to Georg Fritzsche [:gfritzsche] [slow response until Dec 14] from comment #12)
> Comment on attachment 8696843 [details] [diff] [review]
> patch1230213.patch
> 
> Review of attachment 8696843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/tests/unit/test_TelemetryLog.js
> @@ +28,3 @@
> >  {
> > +  do_get_profile();
> > +   yield TelemetrySession.setup();
> 
> This line is now indented three spaces, an oversight?
> It should be indented by two spaces.
> Looks good otherwise :)

Thats funny because when I open the patch file and even the attachment it's only indented by two spaces..
I didn't change the indentation of that line.

Comment 14

[Salah]

(In reply to Salah from comment #13)
> (In reply to Georg Fritzsche [:gfritzsche] [slow response until Dec 14] from
> comment #12)
> > Comment on attachment 8696843 [details] [diff] [review]
> > patch1230213.patch
> > 
> > Review of attachment 8696843 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/components/telemetry/tests/unit/test_TelemetryLog.js
> > @@ +28,3 @@
> > >  {
> > > +  do_get_profile();
> > > +   yield TelemetrySession.setup();
> > 
> > This line is now indented three spaces, an oversight?
> > It should be indented by two spaces.
> > Looks good otherwise :)
> 
> Thats funny because when I open the patch file and even the attachment it's
> only indented by two spaces..
> I didn't change the indentation of that line.
I think I may have uploaded the wrong file. Il try that again for you.

Comment 15

[Salah]

Attachment: patch1230213.patch

Comment 16

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8696944 [details] [diff] [review]
patch1230213.patch

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

Looks good

Comment 17

[Alessio Placitelli [:Dexter]]

Salah, I just noted that your patch does not apply cleanly to mozilla central.
Could you please make sure the patch cleanly applies?

Comment 18

[Salah]

Attachment: Patch.Patch

Hmm, This should work.

Comment 19

[Alessio Placitelli [:Dexter]]

Comment on attachment 8697077 [details] [diff] [review]
Patch.Patch

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

This applies correctly, thanks.

Comment 20

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7980dcaf8666

Comment 21

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/30d3b887b4ec0e3536320b4dabd24627f73dfcd5
Bug 1230213 - test_TelemetryLog yields from a non generator function. r=dexter

Comment 22

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/9a5268189d98 - just like the try push, Android 2.3 was perfectly happy with it, but apparently like several other things in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/xpcshell.ini#40 this patch makes the test crash on Android 4.3, https://treeherder.mozilla.org/logviewer.html#?job_id=6173505&repo=fx-team

Comment 23

[Alessio Placitelli [:Dexter]]

ni? myself to keep it on my radar.

Comment 24

[Alessio Placitelli [:Dexter]]

Apparently, this test had been broken since Jan 2015 [0], and that's why it never crashed on Android as the other tests.

Salah, would you mind doing one last trivial change to disable the test on Android?
You would need to change [1] by adding the following lines after [test_telemetryLog.js]

# Bug 1144395: crash on Android 4.3
skip-if = android_version == "18"

Thank you!

[0] - https://hg.mozilla.org/mozilla-central/rev/133d1d99d3e8
[1] - https://dxr.mozilla.org/mozilla-central/rev/c5cb194cc9cb56d742fb3a7a826f0080b0404edc/toolkit/components/telemetry/tests/unit/xpcshell.ini#36

Comment 25

[Alessio Placitelli [:Dexter]]

Unassigning due to inactivity.

Comment 26

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7041b3c47820

Comment 27

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/836571fbba318c2f8fe7bc0ca515c4876cb19018
Bug 1230213 - test_TelemetryLog yields from a non generator function. r=dexter

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/836571fbba31