1.37 KB, patch
|Details | Diff | Splinter Review|
59 bytes, text/x-review-board-request
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.
Created attachment 8375455 [details] [diff] [review] Test for changes of the revision value in payloads, v1
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?
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.
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.
Priority: -- → P4
Whiteboard: [good second bug][lang=js]
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
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
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.
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?
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
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 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+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/dbd71a5eb617 Test for changes of the revision value in payloads , r=chutten
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.