972324 - Test for changes of the revision value in payloads

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P4)

Description

[Roberto Agostino Vitillo (:rvitillo)]

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.

Comment 1

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: Test for changes of the revision value in payloads, v1

Comment 2

[David Teller [:Yoric] - still alive but not very active]

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?

Comment 3

[Mark Reid [: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.

Comment 4

[Chris H-C :chutten]

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.

Comment 5

[akriti verma]

Hi chris, i would like to work on this bug.

Comment 6

[Chris H-C :chutten]

Do you have enough information here to make a start? This bug is less straightforward than your last one.

Comment 7

[akriti verma]

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

Comment 8

[Chris H-C :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.

Comment 9

[akriti verma]

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?

Comment 10

[Chris H-C :chutten]

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 11

[akriti verma]

Attachment: Bug 972324 : Test for changes of the revision value in payloads ,

Review commit: https://reviewboard.mozilla.org/r/230664/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/230664/

Comment 12

[akriti verma]

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

Comment 13

[Chris H-C :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 :)

Comment 14

[Chris H-C :chutten]

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.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbd71a5eb617
Test for changes of the revision value in payloads , r=chutten

Comment 16

[Cristina Coroiu [:ccoroiu]]

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