RCORE-1973 Add role/permissions tests for new bootstrap feature #7675
Conversation
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1094Details
💛 - Coveralls |
…iption store during server initiated boostrap
…o mwb/user-role-change-tests
…o mwb/user-role-change-tests
michael-wb
force-pushed
the
mwb/user-role-change-tests
branch
from
eee7a17
to
f9aec9b
Compare
| @@ -1208,10 +1212,24 @@ void SessionWrapper::on_flx_sync_progress(int64_t new_version, DownloadBatchStat | |||
| return; | |||
| } | |||
| REALM_ASSERT(!m_finalized); | |||
| REALM_ASSERT(new_version >= m_flx_last_seen_version); | |||
There was a problem hiding this comment.
shouldn't new_version always be either m_flx_last_seen_version, m_flx_active_version or m_flx_active_version+1?
There was a problem hiding this comment.
If a query bootstrap is in progress when a server initiated bootstrap takes place (which cancels the current query bootstrap), m_flx_last_seen_version will be m_flx_active_version + 1 while new_version will be m_flx_active_version and this assertion will fail.
There was a problem hiding this comment.
it's still one of the cases I mentioned. I was proposing we update the assert, not keep the deleted one.
There was a problem hiding this comment.
the m_flx_last_seen_version can actually be greater than m_flx_active_version + 1 in the case of a QUERY_ERROR response and it always starts out at 0 when the session is started until the first download message is received. Since the current validation and checking for server-initiated bootstrap logic is valid, I am going to leave this as-is.
There was a problem hiding this comment.
It seems that m_flx_last_seen_version should be updated when we get a query error then. m_flx_active_version is initialized though when the SessionWrapper is actualized. Either way, your call.
There was a problem hiding this comment.
I would update it, but this is a change that might affect progress notifications, which uses this value. I created RCORE-2173 to handle updating this separately if we want to update it.
…o mwb/user-role-change-tests
…e change bootstraps
… check for role change bootstrap as first validation step
…o mwb/user-role-change-tests
| if (!harness) { | ||
| return; // nothing to do | ||
| } | ||
| harness->app()->sync_manager()->wait_for_sessions_to_terminate(); |
There was a problem hiding this comment.
Why do we need to explicitly wait for sessions to terminate?
There was a problem hiding this comment.
I don't think we need to - this was copied over from another test case that reused the harness across its sections. This entire function was removed.
| return; // nothing to do | ||
| } | ||
| harness->app()->sync_manager()->wait_for_sessions_to_terminate(); | ||
| harness.reset(); |
There was a problem hiding this comment.
Do we re-use the harness unique_ptr? Why not just take the unique_ptr by value and let it go out of scope? Why have this function at all?
There was a problem hiding this comment.
A single harness is re-used across the sections within a test_case. I ended up removing this function and just call harness.reset() in the last section.
| case Event::ErrorMessageReceived: | ||
| REQUIRE(cur_state == TestState::start); | ||
| REQUIRE(data.error_info); | ||
| REQUIRE(data.error_info->raw_error_code == 200); |
There was a problem hiding this comment.
should we compare the error_code rather than the raw_error_code?
There was a problem hiding this comment.
This is a ProtocolErrorInfo struct, which doesn't have error_code; only raw_error_code is available.
| state_machina.transition_with([&](TestState cur_state) { | ||
| switch (expected.bootstrap) { | ||
| case BootstrapMode::NoReconnect: | ||
| // Confirm that the session did receive an error and a bootstrap did not occur |
There was a problem hiding this comment.
Where do we confirm that we received an error? Should the name of this enum value be NoBootstrapNoError or something? Not sure what NoReconnect actually means here.
There was a problem hiding this comment.
Renamed to NoErrorNoBootstrap and updated comment
| REQUIRE_FALSE(role_change_bootstrap); | ||
| break; | ||
| case BootstrapMode::None: | ||
| // Confirm that a bootstrap nor a client reset did not occur |
There was a problem hiding this comment.
So this is something like NoBootstrapReceivedError? "Confirm that neither a bootstrap nor a client rest did not occur" seems like a double negative?
There was a problem hiding this comment.
Renamed state to GotErrorNoBootstrap and updated comment
| update_perms_and_verify(*harness, realm_1, test_rules, | ||
| {BootstrapMode::Any, params.num_emps, params.num_mgrs, 0}); | ||
|
|
||
| // Realm 2 data should not change (and there shouldn't be any bootstrap messages) |
There was a problem hiding this comment.
If there shouldn't be any bootstrap messages, why do we pass BootstrapMode::Any to update_perms_and_verify above?
There was a problem hiding this comment.
I added a comment for this - realm_1 should receive a role change bootstrap, but realm_2 should not (and there is a on_sync_client_event_hook fcn for realm_2 that verifies this).
| update_perms_and_verify(*harness, realm_1, test_rules, | ||
| {BootstrapMode::Any, params.num_emps, params.num_mgrs, params.num_dirs}); | ||
|
|
||
| // Realm 2 data should not change (and there shouldn't be any bootstrap messages) |
There was a problem hiding this comment.
If there shouldn't be any bootstrap messages, why do we pass BootstrapMode::Any to update_perms_and_verify above? Or am I mis-understanding the meaning of BootstrapMode::Any?
There was a problem hiding this comment.
BootstrapMode::Any means that a bootstrap is expected, but we don't care what type of bootstrap (single message, multi message, etc.) occurs.
| if (cur_state == BootstrapTestState::not_ready) | ||
| return std::nullopt; | ||
|
|
||
| std::optional<BootstrapTestState> new_state = std::nullopt; |
There was a problem hiding this comment.
this will default initialize to std::nullopt.
|
|
||
| case Event::ErrorMessageReceived: | ||
| REQUIRE(data.error_info); | ||
| if (data.error_info->raw_error_code == 200) { |
There was a problem hiding this comment.
are there other errors we could receive here that we don't want to track?
There was a problem hiding this comment.
Not really - changed the if to a REQUIRE()
| break; | ||
|
|
||
| case Event::ErrorMessageReceived: | ||
| REQUIRE(data.error_info); |
There was a problem hiding this comment.
I think we can just assume the sync client will do the right thing here.
| return harness; | ||
| } | ||
|
|
||
| void teardown_harness(std::unique_ptr<FLXSyncTestHarness>& harness) |
There was a problem hiding this comment.
I think this should not be a reference and instead pass the ownership because it's not expected to use the harness after calling this function.
There was a problem hiding this comment.
I actually removed this function.
…o mwb/user-role-change-tests
What, How & Why?
Moved the role change bootstrap tests to their own file and reduced the number of times the App was created and destroyed during the tests.
Additional tests for the server initiated bootstraps; specifically for role change bootstraps:
Fixes #7327
☑️ ToDos
[ ] C-API, if public C++ API changed[ ]bindgen/spec.yml, if public C++ API changed