Closed Bug 1304426 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Telemetry, defect, P3)

defect
Points:
1

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: Dexter, Assigned: vi.le, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client] [lang=yaml])

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 1275517
Points: --- → 1
Priority: -- → P3
Whiteboard: [measurement:client]
Mentor: alessio.placitelli
Whiteboard: [measurement:client] → [measurement:client] [lang=yaml]
Hello, can I work on it please ? :-)
(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
Assignee: nobody → sky
Attached patch move-test-scalars.patch (obsolete) — Splinter Review
Attachment #8794126 - Flags: review?(alessio.placitelli)
Thanks :-)
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:
> +      - telemetry-client-dev@mozilla.com
> +    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?
Attachment #8794126 - Flags: review?(alessio.placitelli) → feedback+
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?
(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 |+ |?
Attached patch move-test-scalars.patch (obsolete) — Splinter Review
Attachment #8794126 - Attachment is obsolete: true
Attachment #8795296 - Flags: review?(alessio.placitelli)
Comment on attachment 8795296 [details] [diff] [review]
move-test-scalars.patch

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

This looks good now, thanks!
Attachment #8795296 - Flags: review?(alessio.placitelli) → review+
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.
Status: NEW → ASSIGNED
Flags: needinfo?(sky)
Attachment #8795296 - Attachment is obsolete: true
Attachment #8795416 - Flags: review?(alessio.placitelli)
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!
Flags: needinfo?(sky)
Attachment #8795416 - Flags: review?(alessio.placitelli) → review+
https://hg.mozilla.org/mozilla-central/rev/db9a86ba0a4d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.