Begin decoupling sync tests from App #7351
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tgoyne
force-pushed
the
tg/test-refactoring
branch
from
5dc6089
to
d5713a4
Compare
tgoyne
force-pushed
the
tg/test-refactoring
branch
from
d5713a4
to
8560b50
Compare
Pull Request Test Coverage Report for Build thomas.goyne_179Details
💛 - Coveralls |
tgoyne
force-pushed
the
tg/test-refactoring
branch
from
8560b50
to
15eb07c
Compare
Co-authored-by: James Stone <james.stone@mongodb.com>
tgoyne
force-pushed
the
tg/test-refactoring
branch
from
15eb07c
to
280ce60
Compare
| @@ -22,7 +22,7 @@ | |||
| * Fix a minor race condition when backing up Realm files before a client reset which could have lead to overwriting an existing file. ([PR #7341](https://github.com/realm/realm-core/pull/7341)). | |||
|
|
|||
| ### Breaking changes | |||
| * None. | |||
| * SyncManager no longer supports reconfiguring after calling reset_for_testing(). SyncManager::configure() has been folded into the constructor, and reset_for_testing() has been renamed to tear_down_for_testing(). ([PR #7351](https://github.com/realm/realm-core/pull/7351)) | |||
There was a problem hiding this comment.
I wonder if this should be under internals section. Are users supposed to use reset_for_testing?
There was a problem hiding this comment.
It's a breaking change for the SDKs which use it in their tests.
There was a problem hiding this comment.
right. but don't we share the breaking changes with the users?
There was a problem hiding this comment.
The SDKs are the users of core. They should not copy and paste this into their own changelogs.
danieltabacaru
approved these changes
Feb 19, 2024
michael-wb
approved these changes
Feb 20, 2024
| @@ -78,7 +78,7 @@ util::Optional<std::string> to_optional_string(StringData sd) | |||
| std::vector<AuditEvent> get_audit_events(TestSyncManager& manager, bool parse_events = true) | |||
| { | |||
| // Wait for all sessions to be fully uploaded and then tear them down | |||
| auto sync_manager = manager.app()->sync_manager(); | |||
| auto sync_manager = manager.sync_manager(); | |||
There was a problem hiding this comment.
Nice! These changes are definitely a lot cleaner
| auto user = app->sync_manager()->get_user("user", ENCODE_FAKE_JWT("not_a_real_token"), | ||
| ENCODE_FAKE_JWT("also_not_a_real_token"), dummy_device_id); | ||
| TestSyncManager tsm; | ||
| auto user = tsm.fake_user(); |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Another pile of changes extracted from #7300 with the hopes of making it more reviewable.
The primary change here is that TestSyncManager no longer has an App, and a new type OfflineAppSession replaces it for things testing App using a mock transport. Our test types are now:
This removes the ability to reconfigure SyncManager after calling reset_for_testing(). This was something that was originally needed because SyncManager was a singleton, but that hasn't been the case ever since v10 and tests can now just create a new SyncManager. No one appears to have been relying on this. reset_for_testing() has been renamed to tear_down_for_testing() to reflect the change in functionality.
Footnotes
Or won't post-app services split, anyway. Currently the actual implementation is still fairly coupled. ↩