Test for changes of the revision value in payloads

RESOLVED FIXED in Firefox 61

Status

()

defect
P4
normal
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: rvitillo, Assigned: akriti.v10, Mentored)

Tracking

unspecified
mozilla61
x86
All
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [good second bug][lang=js])

Attachments

(2 attachments)

We should have a test to check if the prefix of the revision value in Telemetry's payloads is unchanged in order to avoid to break server-side code that depends on it.
Assignee: nobody → rvitillo
Comment on attachment 8375455 [details] [diff] [review]
Test for changes of the revision value in payloads, v1

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

So what does this guarantee exactly?
Attachment #8375455 - Flags: review?(dteller)
Flags: needinfo?(mreid)
What we're looking to detect is a revision URL that will be rejected by the server code.

Prior to bug 960571 landing, all the revision URLs began with "http://" so the server code validated accordingly.  See:
https://github.com/mozilla/telemetry-server/blob/1cc4dafa2d2906defd351b609c818e60135655cc/revision_cache.py#L30

The change to "https://" caused Telemetry submissions to be rejected due to invalid revision.

The server code has been updated to accept both http and https now:
https://github.com/mozilla/telemetry-server/blob/1b8c1fb963d87dcccb9849f03a3b10587938e6ab/telemetry/revision_cache.py#L31

But we want to make sure that we don't submit revisions that will be rejected on the server side in the future.
Flags: needinfo?(mreid)
Assignee: rvitillo → nobody
The test should probably just check the exact same regex that the server currently does, here: https://github.com/mozilla/telemetry-server/blob/master/telemetry/revision_cache.py#L32

That way we'll have a local test that fails before we commit changes that will break the server.
Mentor: chutten
Priority: -- → P4
Whiteboard: [good second bug][lang=js]
(Assignee)

Comment 5

a year ago
Hi chris, i would like to work on this bug.
Do you have enough information here to make a start? This bug is less straightforward than your last one.
Assignee: nobody → akriti.v10
(Assignee)

Comment 7

a year ago
Hi Chris , i just want to know a few things. Firstly, do we need to make changes only in the 'toolkit/components/telemetry/tests/unit/test_TelemetryPing.js' file. Secondly, how can i test the changes i make to know whether they are working absolutely fine..
thanks
Flags: needinfo?(chutten)
test_TelemetryPing no longer exists, so it's unlikely to be there :)

The checkPayloadInfo function is now used in tests in the test_TelemetrySession.js file, and that should be the only file you need to edit.

To test your changes, use `mach test toolkit/components/telemetry/tests/unit/test_TelemetySession.js`

Also, to ensure your changes meet the style guidelines, run `mach lint` as well.
Flags: needinfo?(chutten)
(Assignee)

Comment 9

a year ago
What exactly do i need to test in the checkPayloadInfo function. I am trying to test 'revision' with a regex but the test fails. Does 'revision' contain the revision url?
about:telemetry will show you the current Telemetry information. You can look at about:telemetry#session-info-tab to see the expected format of a revision url, or you can click on "Raw JSON" to see the entire ping as a JSON object. You'll find revision under payload.info.

Does that help?
Comment hidden (mozreview-request)
(Assignee)

Comment 12

a year ago
Hi Chris,
The 'revision' field is currently blank. Hence, i was not able to test it with my regex. Therefore i have placed a check for this field, where the test is done only if 'revision' contains the revision url. The 'mach test ....'  and 'mach lint' commands give no error.
Please tell me if this is an appropriate solution for this issue. Also let me know if any changes are required in the regex.
Thanks
Flags: needinfo?(chutten)
I think the code looks sensible.

To make the revision field non-blank you need to compile with MOZ_SOURCE_REPO and MOZ_SOURCE_CHANGESET set to the repository and changeset you sourced the source from. 

For instance, for my mozilla-built Nightly, my MOZ_SOURCE_REPO is https://hg.mozilla.org/mozilla-central and MOZ_SOURCE_CHANGESET is 7771df14ea181add1dc4133f0f5559bf620bf976.

On the plus side, we don't actually need to figure this out ourselves, we can get the tryserver to build one for us. I've asked it to do that over here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d37318074478219e6a15c79a16c04ecd27252466

I've also asked it to run the xpcshell tests, which will include the new condition you added.

We'll see what it has to say about things :)
Flags: needinfo?(chutten)

Comment 14

a year ago
mozreview-review
Comment on attachment 8961838 [details]
Bug 972324 : Test for changes of the revision value in payloads ,

https://reviewboard.mozilla.org/r/230664/#review236604

Looks like this patch and its test check out on try. Let's get this into the tree.
Attachment #8961838 - Flags: review?(chutten) → review+

Comment 15

a year ago
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dbd71a5eb617
Test for changes of the revision value in payloads , r=chutten

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dbd71a5eb617
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.