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.

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-4228] Re-enable skipped presence tests #1886

Conversation

maratal
Copy link
Collaborator

@maratal maratal commented Mar 10, 2024

Closes #1876

In this PR I first removed "skipped" from all presence tests and ran it locally. Those that didn't fail ended up in a loop a few times. Those that failed twice or more were skipped again. Two tests failed once (051 and 030) and I left them active. I've also left FLAKY label for visibility (temporarily).

Read commit messages for more. In total 12 (out of 33) presence tests were unskipped.

@github-actions github-actions bot temporarily deployed to staging/pull/1886/features March 10, 2024 14:58 Inactive
@maratal maratal changed the base branch from main to fix/1848-refactoring-presence March 10, 2024 14:58
@github-actions github-actions bot temporarily deployed to staging/pull/1886/jazzydoc March 10, 2024 15:01 Inactive
@maratal maratal force-pushed the fix/1848-refactoring-presence branch from e365faa to b301249 Compare March 10, 2024 15:02
@@ -128,7 +128,7 @@ class RealtimeClientPresenceTests: XCTestCase {
XCTAssertFalse(channel.internal.presence.syncComplete)
}

func skipped__test__010__Presence__ProtocolMessage_bit_flag__when_members_are_present() throws {
func test__FLAKY__010__Presence__ProtocolMessage_bit_flag__when_members_are_present() throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be renaming tests now right? FLAKY keyward can be removed if they are not flaky anymore wdyt

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to do it later when absolutely sure they are alright.

@sacOO7
Copy link
Contributor

sacOO7 commented Mar 12, 2024

I strongly feel we should unskip/fix most of / all of presence tests. Those are needed to make sure protocol 2.0 / no connection serial works as expected. We did the same for ably-java sometime back ably/ably-java#989

Copy link
Contributor

@umair-ably umair-ably left a comment

Choose a reason for hiding this comment

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

I'm happy for the flaky keyword to be removed later

@maratal
Copy link
Collaborator Author

maratal commented Mar 13, 2024

I strongly feel we should unskip/fix most of / all of presence tests. Those are needed to make sure protocol 2.0 / no connection serial works as expected. We did the same for ably-java sometime back ably/ably-java#989

This may require a lot of effort for each test. This PR contains only tests that just started working normally again at some point. So I've created an umbrella issue for the rest of the presence tests - #1889

@maratal maratal merged commit 34a9eeb into fix/1848-refactoring-presence Mar 13, 2024
7 checks passed
@maratal maratal deleted the fix/1876-re-enable-skipped-presence-tests branch March 13, 2024 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Re-enable skipped presence tests
3 participants