-
Notifications
You must be signed in to change notification settings - Fork 25
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
[ECO-4695] Unskip presense tests #1907
Merged
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
…ight sync is discarded).
…AS_PRESENCE bit flag indicating that it will perform a presence sync". It doesn't say that without members it will omit HAS_PRESENCE flag.
…flag instead of requiring its absence (see RTP1).
maratal
force-pushed
the
fix/1889-unskip-presense-tests
branch
from
April 26, 2024 12:04
ba3ddbb
to
9cfc2fa
Compare
maratal
force-pushed
the
fix/1889-unskip-presense-tests
branch
from
April 26, 2024 12:49
9cfc2fa
to
243c074
Compare
…st RTP2c test). Also reduced number of members to decrease test duration which was also unnecessary long.
…nds PRESENT there as well. Compare connections of two messages.
…his clients remain in memory and interacts with later executing tests sometimes failing them, like here for example - https://test-observability.herokuapp.com/repos/ably/ably-cocoa/test_cases/8eae564d-2dab-409c-a49d-65dddcd5b422?branches%5B%5D=fix%2F1848-refactoring-presence-looped Test 038 doesn't contain any "shouldn't be called" assertions.
…(marked them FLAKY).
…ly called in case of no errors. Otherwise it affects execution of the "test_111..." below - https://test-observability.herokuapp.com/repos/ably/ably-cocoa/test_cases/ee46a8e6-e4ed-489c-83c7-648774930f92?branches%5B%5D=fix%2F1889-unskip-presense-tests-looped-4
…cution of the "test__032..." below - https://test-observability.herokuapp.com/repos/ably/ably-cocoa/test_cases/ac44f09c-f521-4bfc-a3d7-58d4f7428c74?branches%5B%5D=fix%2F1889-unskip-presense-tests-looped-4 Also reducing error delay since often callback on ENTER called faster than 0.1 (at least this may be the reason why "shouldn't be called" assertion is being fired every other time).
…on disconnect internal presence map is kept and upon re-connect no synthesized LEAVE event appears to be sent by realtime.
…e title what is already done in test 080, but does cryptic things in its body. For example the comment "// Should remove the "two" member that was added manually because the connectionId // doesn't match and it's not synthesized, it will be re-entered." doesn't make sense to me, because the member added manually shouldn't be removed. Yes, connection after re-connect is different, but spec doesn't says to remove anything (only in case of channel FAILED or DETACHED should reset both presence maps). And the next comment line says that it will be re-entered. Yes, it will be, so it should be kept then, right? I've marked test 080 as testing spec RTP17b.
…rs (syncInProgress vs not syncInProgress).
…nection goes to CONNECTED (also it's called inside members `get` too). Also removed interception of a SYNC message, since it's not needed here - we just check that sync process was started before calling members `get` and then check if it was finished upon `get` callback (at least that's what the name of the test claims should be happening).
More or less tests look good - https://test-observability.herokuapp.com/repos/ably/ably-cocoa/uploads?branches[]=fix%2F1889-unskip-presense-tests-looped-6 I don't see other than periodical timeout errors, which are "normal" for this test suite. @sacOO7 |
sacOO7
approved these changes
May 13, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #1889
Tests look much better now - https://test-observability.herokuapp.com/repos/ably/ably-cocoa/uploads?branches%5B%5D=fix%2F1889-unskip-presense-tests-looped-5&createdBefore=&createdAfter=2024-05-06T22%3A10%3A42.078Z&failureMessage=
Further runs in a loop show that test__110__Presence__get__Query__set_of_params___waitForSync_is_true__should_wait_until_SYNC_is_complete_befo_re_returning_a_list_of_members and test__080__Presence__private_and_internal_PresenceMap_containing_only_members_that_match_the_current_connec_tionId__any_ENTER__PRESENT__UPDATE_or_LEAVE_event_that_matches_the_current_connectionId_should_be_applied_to_this_object needs investigation. Haven't seen them fail that frequently in other loops.