Skip to content
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

Implment OnRecordsSent and delete resend_records within it #3921

Merged
merged 2 commits into from Nov 9, 2019

Conversation

@darkdh
Copy link
Member

darkdh commented Nov 8, 2019

fix brave/brave-browser#6834

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.
@darkdh darkdh self-assigned this Nov 8, 2019
@darkdh darkdh changed the title On sync records sent Implment OnRecordsSent and delete resend_records within it Nov 8, 2019
@darkdh
Copy link
Member Author

darkdh commented Nov 8, 2019

DEPS will be updated when brave/sync#358 is merged

@darkdh darkdh force-pushed the on-sync-records-sent branch from 8de2a85 to b260f23 Nov 8, 2019
@@ -32,6 +32,7 @@ FORWARD_DECLARE_TEST(BraveSyncServiceTest, ClientOnGetInitData);
FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnGetInitData);
FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnSaveBookmarksBaseOrder);
FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnCompactComplete);
FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnRecordsSent);

This comment has been minimized.

Copy link
@AlexeyBarabash

AlexeyBarabash Nov 8, 2019

Contributor

BraveSyncServiceTest doesn't contain the test named OnRecordsSent, looks the actial test is done in (BraveSyncServiceTest, ExponentialResend)
So this line is not required.

This comment has been minimized.

Copy link
@darkdh

darkdh Nov 8, 2019

Author Member

addressed in 73b682f

@@ -139,6 +143,7 @@ class BraveProfileSyncServiceImpl
FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, ClientOnGetInitData);
FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnSaveBookmarksBaseOrder);
FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnCompactComplete);
FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnRecordsSent);

This comment has been minimized.

Copy link
@AlexeyBarabash

AlexeyBarabash Nov 8, 2019

Contributor

The same as above.

This comment has been minimized.

Copy link
@darkdh

darkdh Nov 8, 2019

Author Member

addressed in 73b682f

@darkdh darkdh force-pushed the on-sync-records-sent branch from b260f23 to 73b682f Nov 8, 2019
Copy link
Contributor

AlexeyBarabash left a comment

LGTM 👍

@darkdh darkdh force-pushed the on-sync-records-sent branch 2 times, most recently from 9679b3b to 6603cdc Nov 8, 2019
darkdh added 2 commits Nov 8, 2019
@darkdh darkdh force-pushed the on-sync-records-sent branch from 6603cdc to acd6576 Nov 9, 2019
@darkdh
Copy link
Member Author

darkdh commented Nov 9, 2019

all passed except Android
https://ci.brave.com/job/brave-browser-build-pr/job/on-sync-records-sent/6/flowGraphTable/
Going to manually rebase brave-browser and build Android only again

@darkdh darkdh merged commit 08f05b9 into master Nov 9, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@darkdh darkdh deleted the on-sync-records-sent branch Nov 9, 2019
@darkdh darkdh added this to the 0.74.x - Nightly milestone Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.