Closed Bug 1313573 Opened 4 years ago Closed 4 years ago

Have TPS validate sync pings

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: markh, Assigned: tcsc)

Details

Attachments

(1 file)

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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.