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

WillieHabi · 2024-12-17T22:31:30Z

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 has their connection removed
            - updates stale attendee status to 'Disconnected' after 30s delay upon reconnection
            - does not update stale attendee status if local client does not reconnect
            - does not update stale attendee status if local client reconnection lasts less than 30s
            - does not update attendee status to 'Disconnected' if stale attendee becomes active
            - updates stale attendee status to 'Disconnected' only 30s after most recent reconnection

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

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 · 2024-12-18T18:19:35Z

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

jason-ha

I like a lot - still have notes :)

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

packages/framework/presence/src/test/presenceManager.spec.ts

jason-ha · 2024-12-18T23:59:54Z

packages/framework/presence/src/test/presenceManager.spec.ts

+						});
+
+						it.skip("updates stale attendees status to 'Disconnected' afer multiple reconnects", () => {


between these cases I am curious what happens after local connect is disconnected again. Do the stale connections ever become disconnected?
And what about the case where local disconnects, connects, and we hear from one of the stale connections?

I added a few other test cases to the existing ones:

stale attendee status is not updated to 'Disconnected' if local client does not reconnect

stale attendee status is not updated to 'Disconnected' if local client reconnection lasts less than 30s

attendee status is not updated to 'Disconnected' if stale attendee becomes active

jason-ha

few more suggestions

jason-ha · 2024-12-20T01:45:44Z

packages/framework/presence/src/test/presenceManager.spec.ts

+						});
+						it("is announced via `attendeeDisconnected`", () => {


nit: Should have a blank line in here

jason-ha · 2024-12-20T01:50:12Z

packages/framework/presence/src/test/presenceManager.spec.ts

+							assert.strictEqual(
+								disconnectedAttendees.length,
+								1,
+								"Exactly one attendee should be announced as disconnected",
+							);
 							assert(
-								disconnectedAttendee !== undefined,
+								disconnectedAttendees[0] !== undefined,
 								"No attendee was disconnected during `removeMember`",
 							);


Second assert is not useful.

jason-ha · 2024-12-20T02:05:48Z

packages/framework/presence/src/test/presenceManager.spec.ts

+							// Verify - stale attendee should still be 'Connected' after 30 seconds
+							clock.tick(30_001);


the tick is part of the action
you can use a larger number (a much larger number)
in the other cases the time past is 31,002.

jason-ha · 2024-12-20T02:19:36Z

packages/framework/presence/src/test/presenceManager.spec.ts

+							clock.tick(15_001); // Wait another 15 seconds (30s total since reconnection)
+
+							// Verify - stale attendee should still be 'Connected' after 30s post-reconnection
+							assert.strictEqual(
+								knownAttendee.getConnectionStatus(),
+								SessionClientStatus.Connected,
+								"Stale attendee should still be 'Connected' after 30s",
+							);


Can use a larger number here as well. There is no time that is too large as should never disconnect.

jason-ha · 2024-12-20T02:24:18Z

packages/framework/presence/src/test/presenceManager.spec.ts

+							// Verify
+							assert.strictEqual(
+								knownAttendee.getConnectionStatus(),
+								SessionClientStatus.Connected,
+								"Active attendee should still be 'Connected' 30s after reconnection",
+							);


And also doesn't raise an attendeeJoined event

jason-ha · 2024-12-20T08:46:42Z

packages/framework/presence/src/test/presenceManager.spec.ts

 					describe("and has their connection removed", () => {
-						it("is announced via `attendeeDisconnected`", () => {


I just realized the grammar of what these cases are saying after rereading the PR description. Thank for putting the test list in there. If you read the "sentence" down you get:
PresenceManager , when connected, attendee that is already known and has their connection removed...
In that situation "their" refers to "attendee". But you mean to refer to this local client.

jason-ha · 2024-12-20T08:49:34Z

packages/framework/presence/src/test/presenceManager.spec.ts

+						it.skip("does not update attendee status to 'Disconnected' if stale attendee becomes active", () => {
+							assert(knownAttendee !== undefined, "No attendee was set in beforeEach");
+
+							// Act - disconnect, reconnect, then process rejoin signal from known attendee after 15s
+							runtime.disconnect();
+							clock.tick(1000);
+							runtime.connect("client6");
+							clock.tick(15_001);
+							processJoinSignals([rejoinAttendeeSignal]);


How else might a stale attendee become active. This sounds like situation where the other was also disconnected but has rejoined. What about if the other had always stayed connected?

initial commit

6f2e9bc

Copilot bot review requested due to automatic review settings December 17, 2024 22:31

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 changed the title ~~initial commit~~ test(client-presence): collateral connection and stale connection tests Dec 17, 2024

Copilot AI reviewed Dec 17, 2024

View reviewed changes

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 added 2 commits December 17, 2024 17:38

comments

d0285b0

stale connection tests

70947e5

WillieHabi requested a review from jason-ha December 17, 2024 23:15

WillieHabi added 2 commits December 18, 2024 12:37

comments

9dc9864

mockEphemeralRuntime changes

16ee7b2

jason-ha reviewed Dec 19, 2024

View reviewed changes

requested changes

cbf9a50

WillieHabi requested a review from jason-ha December 19, 2024 17:51

jason-ha approved these changes Dec 20, 2024

View reviewed changes

jason-ha reviewed Dec 20, 2024

View reviewed changes

jason-ha self-requested a review December 20, 2024 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

WillieHabi commented Dec 17, 2024 •

edited

Loading

WillieHabi commented Dec 18, 2024

jason-ha left a comment

jason-ha Dec 18, 2024

WillieHabi Dec 19, 2024

jason-ha left a comment

jason-ha Dec 20, 2024

jason-ha Dec 20, 2024

jason-ha Dec 20, 2024

jason-ha Dec 20, 2024

jason-ha Dec 20, 2024

jason-ha Dec 20, 2024

jason-ha Dec 20, 2024

		});

		it.skip("updates stale attendees status to 'Disconnected' afer multiple reconnects", () => {

		// Verify - stale attendee should still be 'Connected' after 30 seconds
		clock.tick(30_001);

		describe("and has their connection removed", () => {
		it("is announced via `attendeeDisconnected`", () => {

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

Are you sure you want to change the base?

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

Conversation

WillieHabi commented Dec 17, 2024 • edited Loading

Description

Choose a reason for hiding this comment

WillieHabi commented Dec 18, 2024

jason-ha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jason-ha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillieHabi commented Dec 17, 2024 •

edited

Loading