1304426 - Move test scalars at the bottom of the Scalars.yaml file

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Alessio Placitelli [:Dexter]]

To make easier to navigate the Scalars.yaml file, we should move the telemetry test probes [1] at the end of it, after the real probes.

Comment 1

[Vincent Lequertier]

Hello, can I work on it please ? :-)

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to sky from comment #1)
> Hello, can I work on it please ? :-)

Sure, I've just assigned the bug to you!

Basically, you have to move this group [1] of probes after the last probe in the same file. Once you've done that and built Firefox, to make sure that everything is still working, please run the following command:

./mach test toolkit/components/telemetry

Do not hesitate to ask if you've got other questions or anything is unclear!

[1] - https://dxr.mozilla.org/mozilla-central/rev/560b2c805bf7bebeb3ceebc495a81b2aa4c0c755/toolkit/components/telemetry/Scalars.yaml#4
[2] - https://dxr.mozilla.org/mozilla-central/rev/560b2c805bf7bebeb3ceebc495a81b2aa4c0c755/toolkit/components/telemetry/Scalars.yaml#153

Comment 3

[Vincent Lequertier]

Attachment: move-test-scalars.patch

Comment 4

[Vincent Lequertier]

Thanks :-)

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8794126 [details] [diff] [review]
move-test-scalars.patch

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

Good job Vincent, the patch looks good. There's only one change that should be performed before moving on: could you make sure that the end of the file contains a single blank line, not two, and then post the updated patch?

::: toolkit/components/telemetry/Scalars.yaml
@@ +150,5 @@
> +    kind: uint
> +    notification_emails:
> +      - [e-mail]
> +    release_channel_collection: opt-out
> +

The file now has two blank lines at its end. Would you mind removing one blank line, just leaving a single blank line at the end of the file?

Comment 6

[Vincent Lequertier]

Thank you for the review. I'm not sure to understand. My file has only one blank line at its end, I've double-checked, and the resulting patch (with hg qref) is exactly the same as the one I've attached to the bug. What I am missing?

Comment 7

[Alessio Placitelli [:Dexter]]

(In reply to sky from comment #6)
> Thank you for the review. I'm not sure to understand. My file has only one
> blank line at its end, I've double-checked, and the resulting patch (with hg
> qref) is exactly the same as the one I've attached to the bug. What I am
> missing?

Mh, not sure why you aren't getting the additional blank line, but after applying your patch I get two blank lines at the end :) Could you change the patch so that the last line is |+    release_channel_collection: opt-out| and not |+ |?

Comment 8

[Vincent Lequertier]

Attachment: move-test-scalars.patch

Comment 9

[Alessio Placitelli [:Dexter]]

Comment on attachment 8795296 [details] [diff] [review]
move-test-scalars.patch

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

This looks good now, thanks!

Comment 10

[Alessio Placitelli [:Dexter]]

Would you kindly rebase the patch against the latest m-c? Unfortunately, something changed Scalars.yaml and your patch doesn't apply cleanly any more.

Comment 11

[Vincent Lequertier]

Attachment: move-test-scalars.patch

Comment 12

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db9a86ba0a4dccc83de49febad5ccb67639d8ad0
Bug 1304426 - Move test scalars at the bottom of the Scalars.yaml file. r=dexter

Comment 13

[Alessio Placitelli [:Dexter]]

Comment on attachment 8795416 [details] [diff] [review]
move-test-scalars.patch

Since this is only a rebasing, it doesn't need a new review.
Moreover, this is only swapping the data in the Scalars.yaml file, and was tested locally. No need for a try push.

I've pushed it for you to m-c. Thank you for your patch!

Comment 14

[Carsten Book [:Tomcat]]

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