Closed
Bug 1313573
Opened 8 years ago
Closed 8 years ago
Have TPS validate sync pings
Categories
(Firefox :: Sync, defect, P1)
Firefox
Sync
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).
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → tchiovoloni
Priority: -- → P1
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years ago
|
||
mozreview-review |
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.
Reporter | ||
Comment 3•8 years ago
|
||
mozreview-review |
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
Comment 5•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in
before you can comment on or make changes to this bug.
Description
•