RCORE-1974 Add tests for role/permissions changed during client reset #7840
Conversation
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1194Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1196Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1202Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1220Details
💛 - Coveralls |
|
This PR is waiting for #7850 before it will be posted for review. |
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1234Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1240Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1248Details
💛 - Coveralls |
michael-wb
force-pushed
the
mwb/role-change-client-reset-tests
branch
from
39c8e6e to
9807a39
Compare
…o mwb/role-change-client-reset-tests
…o mwb/role-change-client-reset-tests
| return std::nullopt; | ||
|
|
||
| // Record that the error occurred | ||
| if (data.event == Event::ErrorMessageReceived) { |
There was a problem hiding this comment.
why don't you handle the errors in the switch below?
There was a problem hiding this comment.
Because there is an early return if the update_role_state is empty, which means the role change has been performed and the test doesn't care about the current state anymore. The errors are handled above this since the 200 error will come after the role change has been triggered.
I'll add an early return in this if block, since it doesn't need to continue if an error is received.
…o mwb/role-change-client-reset-tests
| auto set_expected_role_state = [&](ClientResetTestState state, bool skip_role_check = false) { | ||
| client_reset_state.transition_with([&](ClientResetTestState) { | ||
| update_role_state = state; | ||
| // If the role change check is skipped, the test will not look for the role change error |
There was a problem hiding this comment.
Do we want to track the role change bootstrap (where it makes sense) instead? we only seem to track the bootstrap of the client reset session.
There was a problem hiding this comment.
Based on the differing order of hook events depending on where the role change occurs, it is difficult to detect which bootstrap is the role change bootstrap (and sometimes it happens on the fresh realm session, while other times it happens on the primary session). Instead, the test is looking for the 200 error, which should be triggered by the role change, but it is sometimes hard to catch this using the hook event callback, especially when the session is being torn down.
There was a problem hiding this comment.
The tracking of the client reset fresh realm bootstrap is so we can trigger the role change at different points along the client reset chain of events.
| }); | ||
| }; | ||
|
|
||
| SECTION("Bind before client reset") { |
There was a problem hiding this comment.
tbh, i don't even know what these are testing.
There was a problem hiding this comment.
These sections are setting up the state where the role change will take place during the client reset. The entire test case will be executed for each one of these sections.
| case Event::DownloadMessageIntegrated: | ||
| if (!client_reset_error) | ||
| break; | ||
| new_state = ClientResetTestState::cr_session_integrating; |
There was a problem hiding this comment.
a lot of these states just seem to advance to the next state without doing anything. we check below that we reach these states, but i don't know why or what reaching the cr_session_integrating state means with respect to the behavior of this feature.
There was a problem hiding this comment.
Yes - the current phase during the client reset is tracked so the role change can be performed when it reaches the state specified by update_role_state. I will add comments to clarify what each state means.
…o mwb/role-change-client-reset-tests
Yes - this is verifying that if a role change happens at any point during a client reset that the data will eventually be correct once the client reset is finished. |
| bool success = !wait_for_download(*realm); | ||
| success = success && !wait_for_upload(*realm); | ||
| if (!success) | ||
| FAIL("Failed to update realm"); |
There was a problem hiding this comment.
This will produce the same data races that REQUIRE would. Catch2 isn't internally thread-safe, so we have to ensure that we're only using it from one thread at a time.
There was a problem hiding this comment.
Now it is returning the wait_for_download() or wait_for_upload() result if it fails and the bool is checked under a mutex where needed.
| // Session is not valid anymore... exit now | ||
| return SyncClientHookAction::NoAction; |
There was a problem hiding this comment.
Is this expected to happen? The hook function getting called after the SyncSession is destroyed seems like it'd be indicating a bug somewhere.
There was a problem hiding this comment.
It's not supposed to happen - I updated it to a REQUIRE(session)
| SECTION("Client reset session ident") { | ||
| logger->debug("ROLE CHANGE: Role change after client reset session IDENT"); | ||
| // Trigger the role change just after the IDENT message is sent for the fresh realm | ||
| // download session. |
There was a problem hiding this comment.
This is three different descriptions of what the test is testing, with the most important one being the least useful description. It's okay for SECTION titles to be pretty long-winded, and if you're finding it useful to log information in each section it's a pretty strong sign that you should just put that information in the section name. Some of these comments have some details that make sense as a comment, but for a lot of them a slightly reworded version of the comment should just be the section name.
There was a problem hiding this comment.
I cleaned up the section names and the comments - I left the (redundant) debug statements in since it makes it easier to track down which test was being run when the failure occurred - but they have been updated to return the same text as the section name.
…o mwb/role-change-client-reset-tests
* RCORE-1872 Sync client should allow server bootstrapping at any time (#7440) * First round of changes for server-initiated bootstraps * Added test for role change bootstraps * Updated test for handle role bootstraps * Updated baas/baasaas to use branch with fixes * Updated test to verify bootstrap actually occurred * Fixed tsan warning * Updates from review; added comments to clarify bootstrap detection logic * Reverted baas branch to master and protocol version to 12 * Added comments to changes needed when merging to master; update baas version to not use master * Pulled over changes from other branch and tweaking download params * Refactored tests to validate different bootstrap types * Updated tests to get passing using the server params * Updated to support new batch_state protocol changes; updated tests * Updated role change tests and merged test from separate PR * Fixed issue with flx query verion 0 not being treated as a bootstrap * Cleaned up the tests a bit and reworked query version 0 handling * Updates from review; updated batch_state for schema bootstraps * Removed extra mutex in favor of state machine's mutex * Increased timeout when waiting for app initial sync to complete * Updated role change test to use test commands * Update resume and ident message handling * Updated future waits for the pause/resume test command * Added session connected event for when session multiplexing is disabled * Added wait_until() to state machine to wait for callback; updated role change test * RCORE-1973 Add role/permissions tests for new bootstrap feature (#7675) * Moved role change tests to separate test file * Fixed building of new flx_role_change.cpp file * Added local changes w/role bootstrap test - fixed exception in subscription store during server initiated boostrap * Updated local change test to include valid offline writes during role change * Added role change test during initial schema bootstrap * Wrapped up role change during bootstrap tests * Removed debug statments to fix thread sanitizer * Updated sub state comments and reverted a minor change * Refactored role change tests and broke out into 2 separate test cases * Moved harness from a global to a static var in each test case * Reverted resetting the bootstrapping subscription state back to Pending * Updated baas to use protocol v14 and removed the feature flag for role change bootstraps * Updated baasaas version to be a cached version * Updated baasaas githash and reordered role change during bootstrap to check for role change bootstrap as first validation step * Minor updates to reuse the verify_records() fcn * RCORE-2174 Bootstrap store is not being reset if initial subscription bootstrap is interrupted by role change (#7831) * Updated pending bootstrap store to be processed (applied or cleared) when the session or connection is restarted without restarting the Sync Session * RCORE-1974 Add tests for role/permissions changed during client reset (#7840) * re-applied changes after base branch was merged to feature branch * Updates to address test failures * Disable role change check during fresh realm download * Updated comments for clarity * Updates from review; added a bunch of comments to test * Updates to role change tests per review comments * removed ostream support for SyncClientHookEvent
What, How & Why?
This PR builds upon the existing PR #7675 and adds tests to verify the role change bootstraps that occur due to a role change while the sync session is currently processing a client reset.
Fixes #7328
☑️ ToDos
[] 📝 Changelog update[ ] C-API, if public C++ API changed[ ]bindgen/spec.yml, if public C++ API changed