Closed Bug 1223423 Opened 4 years ago Closed 4 years ago

Remove |testOnly| from |TelemetryController.submitExternalPing|

Categories

(Toolkit :: Telemetry, defect, P4)

defect
Points:
1

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: Dexter, Assigned: tomzhang94, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 2 obsolete files)

In [0] we declared a test variable which was just used to debug a problem a while back.

We should drop |const testOnly =| and directly return |Impl.submitExternalPing(...)|.

To test that everything works as expected:

./mach xpcshell-test toolkit/components/telemetry/

[0] - https://dxr.mozilla.org/mozilla-central/rev/4a7526d26bd47ce2e01f938702b91c95424026ed/toolkit/components/telemetry/TelemetryController.jsm#228
Blocks: 1201022
Points: --- → 1
Priority: -- → P4
Whiteboard: [measurement:client][lang=js][good first bug]
Attached patch First Bug Patch (obsolete) — Splinter Review
Hi guys, I'm new to open source and Mozilla and I would like to fix this bug. It seemed pretty trivial so I already gave it a shot with this patch. Let me know if its alright and I interpreted it correctly.
Attachment #8685883 - Flags: review?(alessio.placitelli)
Assignee: nobody → tomzhang94
Comment on attachment 8685883 [details] [diff] [review]
First Bug Patch

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

Hi Tom, welcome and congratulation for your first patch!

You indeed interpreted it correctly, it looks good, with only a minor issue.

If you address that and attach the updated patch, then we can move on to testing! :-)

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +224,5 @@
>    submitExternalPing: function(aType, aPayload, aOptions = {}) {
>      aOptions.addClientId = aOptions.addClientId || false;
>      aOptions.addEnvironment = aOptions.addEnvironment || false;
>  
> +    return Impl.submitExternalPing(aType, aPayload, aOptions);;

There's an extra ";" at the end of the line.
Attachment #8685883 - Flags: review?(alessio.placitelli) → feedback+
Attached patch bug1223423.patch (obsolete) — Splinter Review
Whoops sorry about that. Here's another shot at it. When I inspected the file it automatically added another line from the content that I pulled from the repository. Is this to be expected?
Attachment #8685883 - Attachment is obsolete: true
Attachment #8687081 - Flags: review?(alessio.placitelli)
Comment on attachment 8687081 [details] [diff] [review]
bug1223423.patch

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

Tom, thanks for attaching the new patch. No, that's not expected: that line should not be there. You can remove it from the code, save and refresh the patch (hg qref). This should be enough to get the final patch.

Thank you for your efforts!

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +762,5 @@
>          // Purge the pings archive by removing outdated pings. We don't wait for this
>          // task to complete, but TelemetryStorage blocks on it during shutdown.
>          TelemetryStorage.runCleanPingArchiveTask();
>  
> +        Telemetry.asyncFetchTelemetryData(function () {});

This line should not be there.
Attachment #8687081 - Flags: review?(alessio.placitelli) → feedback+
Here's the patch with the line removed. Sorry for causing so much trouble with such a simple change!
Attachment #8687081 - Attachment is obsolete: true
Attachment #8687083 - Flags: review?(alessio.placitelli)
Comment on attachment 8687083 [details] [diff] [review]
First Bug Patch v3

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

No trouble at all! This looks good now, congratulation on your first r+!

Did you make sure all the test are passing as suggested in the bug description? If not, please kindly check that as well!

I'm pushing it to our try servers for you. We'll land this as soon as the results are in!
Attachment #8687083 - Flags: review?(alessio.placitelli) → review+
Keywords: checkin-needed
Hi Alessio,

Sorry that I didn't mention it but yes, I did run the tests before submitting. It seems that everything integrated without an issue. Should I now set the status of the case to resolved? Furthermore, I was wondering if you could guide me on next steps. Do you have any recommendations on any further bugs with Telemetry? I haven't really had a chance to really dive into the code yet!
https://hg.mozilla.org/mozilla-central/rev/f4ebed413317
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.