-
Notifications
You must be signed in to change notification settings - Fork 941
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sync duplicated object id migration (3) #5139
Conversation
865ae19
to
afc5311
Compare
2aeafd1
to
6b79383
Compare
6b79383
to
309ea38
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed changes - works great 😄 I added a check for the Sync feature flag and then added a unit test for this too
309ea38 LGTM |
…icated folders; fixes brave-browser#8358
…ding nested in duplicated folders
- Prevents brave/brave-browser#9110 (or similar) from happening - Added unit test to ensure migration not done if sync disabled
309ea38
to
b631e74
Compare
CI had a failure; deleted auto-created |
CI passed on all platforms, except for Android. Seems there was a signing issue during |
Android is building fine, tests pass- there's just a problem with @iefremov this should fix some of the crashes you've seen reported, I believe 😄 |
LGTM Verification passed on
Verified test plan from #5139 Steps used for brave/brave-browser#8358
Reproduced the crash after upgrade to 1.7.90 Test case - disabled sync
Verified no crash after upgrade to 1.9.15 Verified test plan from brave/brave-browser#9110 Reproduced the crash after upgrade to 1.7.90 |
Resolves brave/brave-browser#8358
Resolves brave/brave-browser#9110
Submitter Checklist:
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
See brave/brave-browser#8358
Also see brave/brave-browser#9110
Reviewer Checklist:
After-merge Checklist:
changes has landed on.