Closed Bug 1427835 Opened 2 years ago Closed 2 years ago

Fix or delete failing TPS tests around non-restartless addons

Categories

(Firefox :: Sync, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: tcsc, Assigned: tcsc)

References

Details

Attachments

(1 file)

At the moment it fails in automation because of bug 1380472.

This could be solved by disabling the validator (a la 1382044), or by using a restartless addon for it.

The second sounds like a good idea, but afterwards the test would literally become just "install an addon, sync, restart, and ensure it's still there -- this test only runs on a single profile and with two phases: https://searchfox.org/mozilla-central/source/services/sync/tests/tps/test_addon_sanity.js

Note that TPS has a number of other more robust addon tests that already test the more involved cases across multiple profiles, for both restartless and non-restartless (arguably, the latter is worth removing, but probably not until we decide to take the complexity around non-restartless addons out of sync entirely), so deleting this test should not reduce test coverage.

Trivial patch incoming.
test_addon_nonrestartless.js also fails, so I'm going to fix that too, but just by disabling the validator in those phases (since deleting this seems like it will reduce test coverage in a meaningful way).

It seems like the problem in bug 1380472 got worse while the other TPS tests were failing, but only (AFAICT) on linux. It's at the point that installing a non-restartless addon, syncing, restarting, and syncing again will end up with two syncGUIDs on the server. This *probably* doesn't matter, since we dont' really care about non-restartless addons, but we still have code to support them (To be honest, I'm not sure how much, but it's clearly not none), so... It's not clear that deleting this test is the right choice while that's around.

Anyway, it's unfortunately somewhat hard to know how long ago this started, since it has been failing for some time, so I've filed https://github.com/mozilla-services/sync-tps-setup/pull/9 to address it.
Summary: Delete TPS test test_addon_sanity.js → Fix or delete failing TPS tests around non-restartless addons
Comment on attachment 8939628 [details]
Bug 1427835 - Remove redundant TPS addon test and disable validation on the non-restartless XPI test

https://reviewboard.mozilla.org/r/209932/#review215586

LGTM, but I think we should get another bug on file (or reopen that other one) do we understand what is going on here and can reinstate the validator - mainly I'm skeptical that it's been broken forever, so I suspect something else (eg, that we broke is recently, but via something like the async work introducing a race condition rather than via something we knowingly chaged in addon syncing)
Attachment #8939628 - Flags: review?(markh) → review+
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ebf0ff5a15a
Remove redundant TPS addon test and disable validation on the non-restartless XPI test r=markh
https://hg.mozilla.org/mozilla-central/rev/2ebf0ff5a15a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.