1223423 - Remove |testOnly| from |TelemetryController.submitExternalPing|

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P4)

Description

[Alessio Placitelli [:Dexter]]

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

Comment 1

[Tom Zhang]

Attachment: First Bug Patch

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.

Comment 2

[Alessio Placitelli [:Dexter]]

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.

Comment 3

[Tom Zhang]

Attachment: bug1223423.patch

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?

Comment 4

[Alessio Placitelli [:Dexter]]

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.

Comment 5

[Tom Zhang]

Attachment: First Bug Patch v3

Here's the patch with the line removed. Sorry for causing so much trouble with such a simple change!

Comment 6

[Alessio Placitelli [:Dexter]]

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!

Comment 7

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e580fe9f04f

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f4ebed413317

Comment 9

[Tom Zhang]

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!

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f4ebed413317