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.

Sign up for GitHub

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

Jump to bottom

Leave sync chain after 5 seconds without ack response #10044

Merged
merged 2 commits into from
Sep 17, 2021
Merged

Leave sync chain after 5 seconds without ack response #10044

merged 2 commits into from
Sep 17, 2021

Conversation

AlexeyBarabash
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash commented Sep 10, 2021 •
edited
Loading

Resolves brave/brave-browser#18054

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  1. new profile
  2. launch Brave
  3. click on the “hamburger” menu
  4. choose Sync
  5. click on Start a new Sync Chain
  6. click on Computer
  7. click OK
  8. Turn off internet
  9. go to Manage your synced devices (brave://settings/braveSync/setup)
  10. click Leave Sync Chain,
  11. device should leave sync in 5 seconds,
  12. open in brave://sync-internals, it should have Transport State Disabled.

@AlexeyBarabash AlexeyBarabash added feature/sync CI/skip Do not run CI builds (except noplatform) labels Sep 10, 2021
AlexeyBarabash
AlexeyBarabash commented Sep 10, 2021
View reviewed changes
chromium_src/components/sync_device_info/device_info_sync_bridge_unittest.cc

bool deleted_device_info_sent = false;
base::RunLoop loop;
bridge()->DeleteDeviceInfo(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test actually last for 6 seconds. Not sure, is that possible to make it instant.

Copy link
Member

@goodov goodov Sep 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please take a look at base::ScopedMockTimeMessageLoopTaskRunner. it can be used to fast-forward delayed PostTask.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Playing with ScopedMockTimeMessageLoopTaskRunner didn't gave a good result, but scoped_mock_time_message_loop_task_runner.h contains a comment

// Note: Use TaskEnvironment + TimeSource::MOCK_TIME instead of this in unit
// tests.

It turned out DeviceInfoSyncBridgeTest already uses TaskEnvironment so to avoid patches I have to make BraveDeviceInfoSyncBridgeTest with TaskEnvironment(MOCK_TIME) to run LocalDeleteWhenOffline

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about test::TaskEnvironment::FastForwardBy(base::TimeDelta::FromSeconds(6))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, just tried, but this didn't help.
Comments near the FastForwardBy says

// Only valid for instances using TimeSource::MOCK_TIME.

and I got DCHECK(mock_time_domain_); fired. Original task_environment for DeviceInfoSyncBridgeTest is created without MOCK_TIME flag and I have to make a new BraveDeviceInfoSyncBridgeTest with MOCK_TIME - then it doesn't require to call `FastForwardBy.

Copy link
Member

@goodov goodov Sep 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about this?

namespace base {
namespace test {
namespace {

class TaskEnvironmentOptionalMockTime : public TaskEnvironment {
 public:
  TaskEnvironmentOptionalMockTime()
      : TaskEnvironment(
            IsMockTimedTest(
                testing::UnitTest::GetInstance()->current_test_info()->name())
                ? TimeSource::MOCK_TIME
                : TimeSource::DEFAULT) {}

  static bool IsMockTimedTest(base::StringPiece test_name) {
    return test_name == "LocalDeleteWhenOffline";
  }
};

}  // namespace
}  // namespace test
}  // namespace base

#define TaskEnvironment TaskEnvironmentOptionalMockTime

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@goodov this is cool and this works 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushed fix, thanks @goodov

@AlexeyBarabash AlexeyBarabash removed the CI/skip Do not run CI builds (except noplatform) label Sep 13, 2021
@AlexeyBarabash AlexeyBarabash marked this pull request as ready for review September 13, 2021 14:19
@AlexeyBarabash AlexeyBarabash requested review from a team as code owners September 13, 2021 14:19
goodov
goodov reviewed Sep 13, 2021
View reviewed changes
chromium_src/components/sync_device_info/device_info_sync_bridge_unittest.cc

bool deleted_device_info_sent = false;
base::RunLoop loop;
bridge()->DeleteDeviceInfo(
Copy link
Member

@goodov goodov Sep 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please take a look at base::ScopedMockTimeMessageLoopTaskRunner. it can be used to fast-forward delayed PostTask.

chromium_src/components/sync_device_info/device_info_sync_bridge_unittest.cc
ASSERT_EQ(2, change_count());

const std::string kLocalGuid = CacheGuidForSuffix(kLocalSuffix);
ON_CALL(*processor(), IsEntityUnsynced(kLocalGuid))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it's possible to count this calls? Also it seems ok to copy "5" into test and not introduce a public getter for tests. or just make a constant as extern so it can be used in tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I have addressed both this notices.
Calls are counted at EXPECT_CALL(*processor(), IsEntityUnsynced).Times(5); and I replaced the getter with hard-coded value of 5.

@AlexeyBarabash AlexeyBarabash mentioned this pull request Sep 15, 2021
Closed
darkdh
darkdh approved these changes Sep 15, 2021
View reviewed changes
chromium_src/components/sync_device_info/device_info_sync_bridge_unittest.cc
} // namespace test
} // namespace base

#define TaskEnvironment TaskEnvironmentOptionalMockTime

#include "../../../../components/sync_device_info/device_info_sync_bridge_unittest.cc"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#undef TaskEnvironment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed. thanks, fixed.

goodov
goodov reviewed Sep 16, 2021
View reviewed changes
chromium_src/components/sync_device_info/device_info_sync_bridge_unittest.cc Outdated

loop.Run();

EXPECT_EQ(5, bridge()->GetDeviceDeleteAttemptsForTest());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that we can drop this when we started to count IsEntityUnsynced calls. wdyt? it would be a little cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, with that was possible to remove both GetDeviceDeleteAttemptsForTest and device_delete_attempts_for_test_

goodov
goodov approved these changes Sep 16, 2021
View reviewed changes
Copy link
Member

@goodov goodov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approve, but consider removing GetDeviceDeleteAttemptsForTest.

@AlexeyBarabash AlexeyBarabash merged commit 3e1eefb into master Sep 17, 2021
@AlexeyBarabash AlexeyBarabash deleted the sync_leave_when_offline branch September 17, 2021 10:47
@AlexeyBarabash AlexeyBarabash added this to the 1.31.x - Beta milestone Sep 17, 2021
@srirambv srirambv mentioned this pull request Oct 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darkdh darkdh darkdh approved these changes

@goodov goodov goodov approved these changes

Assignees

@AlexeyBarabash AlexeyBarabash

Labels
feature/sync
Projects
None yet
Milestone
1.31.x - Release
Development

Successfully merging this pull request may close these issues.

Allow to leave sync chain when offline
3 participants
@AlexeyBarabash @goodov @darkdh