Closed Bug 1230213 Opened 4 years ago Closed 4 years ago

test_TelemetryLog yields from a non generator function

Categories

(Toolkit :: Telemetry, defect, P3)

defect
Points:
1

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox45 --- affected
firefox46 --- fixed

People

(Reporter: Dexter, Unassigned, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 5 obsolete files)

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
Blocks: 1201022
Points: --- → 1
Priority: -- → P3
Whiteboard: [measurement:client][lang=js][good first bug]
Hi, I would like to work on this bug. Could you give me some starter instructions on how to begin. Thanks
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
Attached patch Bug fix attempt (obsolete) — Splinter Review
Ive made the proposed changes. Not sure exactly where to add the final ).
Attachment #8696364 - Flags: feedback?
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+
Attached patch patch1230213[1].patch (obsolete) — Splinter Review
Tests passed and changes made
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!
Attached patch patch1230213[Final].patch (obsolete) — Splinter Review
Final Changes
Attachment #8696649 - Flags: review?(alessio.placitelli)
Attachment #8696476 - Attachment is obsolete: true
Attachment #8696364 - Attachment is obsolete: true
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+
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.
Attached patch patch1230213.patch (obsolete) — Splinter Review
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?
(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.
(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.
Attached patch patch1230213.patch (obsolete) — Splinter Review
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+
Attachment #8696649 - Attachment is obsolete: true
Attachment #8696843 - Attachment is obsolete: true
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)
Attached patch Patch.PatchSplinter Review
Hmm, This should work.
Flags: needinfo?(meftahs)
Attachment #8697077 - Flags: review?(alessio.placitelli)
Attachment #8697077 - Attachment is patch: true
Attachment #8697077 - Attachment mime type: text/x-patch → text/plain
Attachment #8696944 - Attachment is obsolete: true
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+
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
ni? myself to keep it on my radar.
Flags: needinfo?(alessio.placitelli)
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)
Unassigning due to inactivity.
Assignee: meftahs → nobody
Flags: needinfo?(meftahs)
Depends on: 1136634
https://hg.mozilla.org/integration/fx-team/rev/836571fbba318c2f8fe7bc0ca515c4876cb19018
Bug 1230213 - test_TelemetryLog yields from a non generator function. r=dexter
https://hg.mozilla.org/mozilla-central/rev/836571fbba31
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.