1343093 - Add integration test for bookmark repair

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P2)

Description

[Lina Butler [:lina]]

Attachment: repair-integration-test.patch

As written, the integration test depends on being able to shut down and restart Places with a different database multiple times in the same process. This is complicated to get right. Assuming that's the way to go, we can make Places restartable and land this. If not, we'll need to think of a different strategy.

Comment 1

[Lina Butler [:lina]]

Attachment: Bug 1343093 - Add integration test for bookmark repair.

This test replaces the clients engine to simulate two different
clients, and uses `SOURCE_SYNC` to bypass change tracking.

Review commit: https://reviewboard.mozilla.org/r/122452/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122452/

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8849679 [details]
Bug 1343093 - Add integration test for bookmark repair.

https://reviewboard.mozilla.org/r/122452/#review124728

Awesome, thanks!

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8849679 [details]
Bug 1343093 - Add integration test for bookmark repair.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122452/diff/1-2/

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b88f35ad48cc
Add integration test for bookmark repair. r=markh

Comment 5

[Lina Butler [:lina]]

Comment on attachment 8849679 [details]
Bug 1343093 - Add integration test for bookmark repair.

This is a test-only change, but the new test uncovered a bug in the clients engine, so filing a request instead of "a=testonly".

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1317223.
[User impact if declined]: No user impact. This is a new test for bookmarks repair, with a bug fix for a path that the previous test didn't catch.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: N/A
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Very low risk.
[Why is the change risky/not risky?]: Test-only change.
[String changes made/needed]: None.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b88f35ad48cc

Comment 7

[Gerry Chang [:gchang]]

Comment on attachment 8849679 [details]
Bug 1343093 - Add integration test for bookmark repair.

Improve the test coverage for bookmark repair. Aurora54+.

Comment 8

[Carsten Book [:Tomcat]]

seems this needs rebasing 

grafting 406699:b88f35ad48cc "Bug 1343093 - Add integration test for bookmark repair. r=markh"
merging services/sync/modules/engines/clients.js
merging services/sync/tests/unit/test_bookmark_repair.js
warning: conflicts while merging services/sync/tests/unit/test_bookmark_repair.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/76541e589505

Comment 10

[Lina Butler [:lina]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/76541e589505