Have TPS validate sync pings

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: markh, Assigned: tcsc)

Tracking

unspecified
Firefox 52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

Thom suggested that if TPS validated any sync pings it generates against the schema we would be more likely to see issues which cause us to generate an invalid ping (eg, bug 1313495).
Assignee: nobody → tchiovoloni
Priority: -- → P1
Comment on attachment 8808331 [details]
Bug 1313573 - Validate the sync ping during TPS test runs

https://reviewboard.mozilla.org/r/91160/#review90934

There is some substantially dubious parts of this patch. If you have an idea how to write it in a better way, LMK.

::: services/sync/tests/unit/sync_ping_schema.json:31
(Diff revision 1)
>            "type": "string",
>            "pattern": "^[0-9a-f]{32}$"
>          },
>          "devices": {
>            "type": "array",
> -          "items": { "$ref": "#/definitions/engine" }
> +          "items": { "$ref": "#/definitions/device" }

First bug caught by this feature. Let me know if I should move this into it's own patch.

::: services/sync/tps/extensions/tps/resource/tps.jsm:910
(Diff revision 1)
>     *
>     * This is called by RunTestPhase() after the environment is validated.
>     */
>    _executeTestPhase: function _executeTestPhase(file, phase, settings) {
>      try {
> +      this.config = JSON.parse(prefs.getCharPref('tps.config'));

This used to be written into the test file by the python code, which meant we used a tmpfile. Which of course, breaks the hacky things I do above to find the source root.

While its not ideal to need to do those things, passing this via a pref is much cleaner IMO than having a implicitly added variable that's created via python.

::: services/sync/tps/extensions/tps/resource/tps.jsm
(Diff revision 1)
>        Logger.logInfo("Starting phase " + this._currentPhase);
>  
>        Logger.logInfo("setting client.name to " + this.phases[this._currentPhase]);
>        Weave.Svc.Prefs.set("client.name", this.phases[this._currentPhase]);
>  
> -      // If a custom server was specified, set it now

I removed this as part an initial attempt to get rid of the config variable (which it turns out we do need, hence adding it via a pref).

That said, this code is either unused or broken, depending on which part, so it seems better to remove it alltogether.

::: services/sync/tps/extensions/tps/resource/tps.jsm:985
(Diff revision 1)
>      };
>      SyncTelemetry.submit = record => {
>        Logger.logInfo("Intercepted sync telemetry submission: " + JSON.stringify(record));
>        this._syncsReportedViaTelemetry += record.syncs.length + (record.discarded || 0);
>        if (record.discarded) {
> -        Logger.AssertTrue(record.syncs.length == SyncTelemetry.maxPayloadCount,
> +        if (record.syncs.length != SyncTelemetry.maxPayloadCount) {

These wouldn't actually cause TPS to fail.
Comment on attachment 8808331 [details]
Bug 1313573 - Validate the sync ping during TPS test runs

https://reviewboard.mozilla.org/r/91160/#review90980

Looks great - a bit of a shame we don't have testing-common etc, but as we discussed, it's not worth spending a week on just for TPS.
Attachment #8808331 - Flags: review?(markh) → review+
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c05a3ad32d21
Validate the sync ping during TPS test runs r=markh
https://hg.mozilla.org/mozilla-central/rev/c05a3ad32d21
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.