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

test(client-presence): old collateral connection and stale connection tests #23351

Merged
merged 17 commits into from
Jan 2, 2025

Conversation

WillieHabi
Copy link
Contributor

@WillieHabi WillieHabi commented Dec 17, 2024

Description

There are 6 tests that are added (and disabled) in this PR.

The first test is to verify that old collateral connections do not effect the connection status of remote attendees. This scenario was written as an edge case to old solution where old connection information sent in a join response would result in the attendee connection status being set to 'Disconnected'.

The next five have to do with stale remote attendee's connection status upon local client reconnect scenarios:

Presence
  PresenceManager
    when connected
      attendee
        that is joining
          - as collateral with old connection info and connected is NOT announced via `attendeeJoined
        that is already known
          and then local client disconnects
            - updates status of attendee with stale connection after 30s delay upon local reconnection
            - does not update status of attendee with stale connection if local client does not reconnect
            - does not update status of attendee with stale connection if local client reconnection lasts less than 30s
            - does not update status of attendee with stale connection if attendee rejoins
            - does not update status of attendee with stale connection if attendee sends datastore update
            - marks attendee with stale connection as active when attendee disconnects after local reconnection
            - updates status of attendee with stale connection only 30s after most recent local reconnection

I added connect() and disconnect() function to MockEphemeralRuntime that manipulates the connection state of the local client.

@Copilot Copilot bot review requested due to automatic review settings December 17, 2024 22:31
@github-actions github-actions bot added area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct base: main PRs targeted against main branch labels Dec 17, 2024
@WillieHabi WillieHabi changed the title initial commit test(client-presence): collateral connection and stale connection tests Dec 17, 2024

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

packages/framework/presence/src/internalTypes.ts:54

  • The event names 'connected' and 'disconnected' should be consistent with the rest of the codebase.
export interface EphemeralRuntimeEvents extends IEvent {

packages/framework/presence/src/test/mockEphemeralRuntime.ts:121

  • The removal of the 'on' and 'off' methods from 'MockEphemeralRuntime' might break existing code that relies on these methods. Confirm if this change was intentional and if it might cause any issues.
public clientId: string | undefined;

packages/framework/presence/src/test/mockEphemeralRuntime.ts:71

  • [nitpick] Ensure that the new event handling mechanism is consistent with the rest of the codebase and aligns with existing conventions.
extends TypedEventEmitter<EphemeralRuntimeEvents>
@WillieHabi WillieHabi changed the title test(client-presence): collateral connection and stale connection tests test(client-presence): old collateral connection and stale connection tests Dec 17, 2024
@WillieHabi WillieHabi requested a review from jason-ha December 17, 2024 23:15
@WillieHabi
Copy link
Contributor Author

Updated the tests a bit from our last discussion, looking for some guidance around better ways to simulate reconnect since I now know we want to move away from TypedEventEmitter @jason-ha

Copy link
Contributor

@jason-ha jason-ha left a comment

Choose a reason for hiding this comment

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

I like a lot - still have notes :)

@WillieHabi WillieHabi requested a review from jason-ha December 19, 2024 17:51
Copy link
Contributor

@jason-ha jason-ha left a comment

Choose a reason for hiding this comment

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

few more suggestions

@jason-ha jason-ha self-requested a review December 20, 2024 08:50
@WillieHabi
Copy link
Contributor Author

@jason-ha Thanks for approval, but added some stuff from the feedback that definitely requires additional review before merging:

  • created a separate describe block 'when local client disconnects' to move away from 'and has their connection removed'.
  • created a test case in which a datastore update message activates a stale attendee.
  • made larger advances in time (a minute) before checking for connection status of stale attendees

Copy link
Contributor

@jason-ha jason-ha left a comment

Choose a reason for hiding this comment

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

"stale" attendee needs a better descriptor or a comment in code. There isn't anything explaining what that is. (How do I check that there is also a test case for a non-stale attendee? What is the difference?)
Please also update the test case list in PR description.

@WillieHabi
Copy link
Contributor Author

"stale" attendee needs a better descriptor or a comment in code. There isn't anything explaining what that is. (How do I check that there is also a test case for a non-stale attendee? What is the difference?) Please also update the test case list in PR description.

I added a description of the stale connection scenario before the tests.

I also did not like the wording from "stale attendee" to "attendee with stale connection". We're waiting to change the connection status of the attendee, so it feels like that's the actual part getting "stale". Can switch back if the conciseness if preferred.

@WillieHabi WillieHabi requested a review from jason-ha December 20, 2024 20:46
@WillieHabi WillieHabi requested a review from jason-ha December 20, 2024 22:18
@WillieHabi WillieHabi merged commit 43dc694 into microsoft:main Jan 2, 2025
26 checks passed
WillieHabi added a commit that referenced this pull request Jan 17, 2025
## Description

[ADO Bug
24395](https://dev.azure.com/fluidframework/internal/_workitems/edit/24395)

This PR enables the previously failing tests presence attendee tests as
well as the new ones added in this PR:
#23351

When the local attendee disconnects, we temporarily lose connectivity
status for remote attendees. To fix this we mark all 'Connected' remote
attendee connections as stale upon reconnect and update their status to
disconnected after 30 seconds of inactivity.

This PR also fixes some spelling mistakes: "seassionId-2" to
"sessionId-2"

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants