test_TelemetryLog yields from a non generator function

RESOLVED FIXED in Firefox 46

Status

()

Toolkit
Telemetry
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Dexter, Unassigned, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
mozilla46
Points:
1
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox46 fixed)

Details

(Whiteboard: [measurement:client][lang=js][good first bug])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

2 years ago
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
(Reporter)

Updated

2 years ago
Blocks: 1201022
Points: --- → 1
Priority: -- → P3
Whiteboard: [measurement:client][lang=js][good first bug]

Comment 1

2 years ago
Hi, I would like to work on this bug. Could you give me some starter instructions on how to begin. Thanks
(Reporter)

Comment 2

2 years ago
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!
Assignee: nobody → meftahs

Comment 3

2 years ago
Created attachment 8696364 [details] [diff] [review]
Bug fix attempt

Ive made the proposed changes. Not sure exactly where to add the final ).
Attachment #8696364 - Flags: feedback?
(Reporter)

Comment 4

2 years ago
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.
Attachment #8696364 - Flags: feedback? → feedback+

Comment 5

2 years ago
Created attachment 8696476 [details] [diff] [review]
patch1230213[1].patch

Tests passed and changes made
(Reporter)

Comment 6

2 years ago
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

2 years ago
Created attachment 8696649 [details] [diff] [review]
patch1230213[Final].patch

Final Changes
Attachment #8696649 - Flags: review?(alessio.placitelli)
(Reporter)

Updated

2 years ago
Attachment #8696476 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Attachment #8696364 - Attachment is obsolete: true
(Reporter)

Comment 8

2 years ago
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!
Attachment #8696649 - Flags: review?(alessio.placitelli) → review+
(Reporter)

Comment 9

2 years ago
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c93aabab57dc
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

2 years ago
Created attachment 8696843 [details] [diff] [review]
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]:
Attachment #8696843 - Flags: review?(alessio.placitelli)
Attachment #8696843 - Flags: approval-mozilla-aurora?
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 :)
Attachment #8696843 - Flags: review?(alessio.placitelli)
Attachment #8696843 - Flags: approval-mozilla-aurora?

Comment 13

2 years ago
(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

2 years ago
(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

2 years ago
Created attachment 8696944 [details] [diff] [review]
patch1230213.patch
Attachment #8696944 - Flags: review?(gfritzsche)
Comment on attachment 8696944 [details] [diff] [review]
patch1230213.patch

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

Looks good
Attachment #8696944 - Flags: review?(gfritzsche) → review+
(Reporter)

Updated

2 years ago
Attachment #8696649 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Attachment #8696843 - Attachment is obsolete: true
(Reporter)

Comment 17

2 years ago
Salah, I just noted that your patch does not apply cleanly to mozilla central.
Could you please make sure the patch cleanly applies?
Flags: needinfo?(meftahs)

Comment 18

2 years ago
Created attachment 8697077 [details] [diff] [review]
Patch.Patch

Hmm, This should work.
Flags: needinfo?(meftahs)
Attachment #8697077 - Flags: review?(alessio.placitelli)
(Reporter)

Updated

2 years ago
Attachment #8697077 - Attachment is patch: true
Attachment #8697077 - Attachment mime type: text/x-patch → text/plain
(Reporter)

Updated

2 years ago
Attachment #8696944 - Attachment is obsolete: true
(Reporter)

Comment 19

2 years ago
Comment on attachment 8697077 [details] [diff] [review]
Patch.Patch

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

This applies correctly, thanks.
Attachment #8697077 - Flags: review?(alessio.placitelli) → review+
(Reporter)

Comment 20

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7980dcaf8666
(Reporter)

Comment 21

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/30d3b887b4ec0e3536320b4dabd24627f73dfcd5
Bug 1230213 - test_TelemetryLog yields from a non generator function. r=dexter
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
(Reporter)

Comment 23

2 years ago
ni? myself to keep it on my radar.
Flags: needinfo?(alessio.placitelli)
(Reporter)

Comment 24

2 years ago
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
Flags: needinfo?(alessio.placitelli) → needinfo?(meftahs)
(Reporter)

Comment 25

2 years ago
Unassigning due to inactivity.
Assignee: meftahs → nobody
Flags: needinfo?(meftahs)
(Reporter)

Updated

2 years ago
Depends on: 1136634
(Reporter)

Comment 26

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7041b3c47820
(Reporter)

Comment 27

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

Comment 28

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/836571fbba31
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.