fix(client-presence): disabled (failing) attendees unit tests #23113

WillieHabi · 2024-11-15T22:06:16Z

Description

A few unit tests were added disabled as current implementation does not meet expectations. This PR enable these tests and updates implementation:

Presence
  PresenceManager
    when connected
      attendee
        that is joining
          - second time is announced once via `attendeeJoined` with status "Connected" when prior is still connected
          - first time is announced via `attendeeJoined` with status "Connected" even if unknown to audience
        that is already known
          - is not announced via `attendeeJoined` when already "Connected"`

jason-ha · 2024-11-15T22:21:11Z

packages/framework/presence/src/systemWorkspace.ts

+			const isAttendeeConnected =
+				audienceMembers.has(clientConnectionId) ||
+				senderConnectionId === clientConnectionId ||
+				connectedAttendees.has(attendee);



This feels like a pretty complicated state system. I feel like I'll be able to break it. :)
This line in particular needs more explanation. The other two make sense.

Added some comments here for each condition, but willing to change if one does not seem reasonable

The commentary that is interesting to capture is why the first two checks aren't sufficient.
And perhaps make the statement less strong. Really, we consider the attendee connected if the audience says so. And we override audience when sender id matches (which we want comments for somewhere).
I believe the last case is saying: attendee is connected if any of the connection ids for the attendee are current/connected. As we process in a list this check provides if any of the past connection ids were connected. If none are to this point then we'll set disconnected and should any later connection be found connected status will be set to connected.

Ok yes this makes sense, will add a more detailed comment along these lines

I wanted to also add that I did not make any assumptions based on the ordering of connection IDs received from the remote datastore. That is, I did not write the code with the guarantee that previous connection IDs will appear first, and more recent ones will appear later. The last check is not necessary if that is a guarantee that we enforce, and I think this function can be written without the connectedAttendees set.

packages/framework/presence/src/systemWorkspace.ts

jason-ha · 2024-11-15T22:29:59Z

packages/framework/presence/src/systemWorkspace.ts

 			attendee.setConnectionId(clientConnectionId);
-			isNew = true;
 		}


add comments

maybe isNew could use a rename like isNewConnection.

an overall comment that all updated attendees will have their connection status set to Connected seems worthy.... as it is up to the caller to reset if needed.

I think isJoining (the session) is the best name here, since it dictates if we announce attendeeJoined event. If a known attendee is connected and we receive a new connectionId, we should set the new connectionId but they are not necessarily joining the session and should not be announced. If known attendee is disconnected then it makes sense to announce new connection, since they are (re)joining the session.

Brought back updateStatus to setConnectionId, so we can update connectionId here and handle connection status in processUpdate()

If we don't set connection status here where should setConnectionId be called to silently update the status?
(Silence is bad. With something as tricky as the connection status seems to be anything that updates the status should be very clear and explicit. Default param is worst. Having it be a boolean is secondarily unfortunate.)

See comment #23113 (comment)

packages/framework/presence/src/systemWorkspace.ts

msfluid-bot

Code Coverage Summary

↑ packages.framework.presence.src:
Line Coverage Change: 0.04% Branch Coverage Change: 0.56%

Metric Name	Baseline coverage	PR coverage	Coverage Diff
Branch Coverage	88.57%	89.13%	↑ 0.56%
Line Coverage	73.96%	74.00%	↑ 0.04%

Baseline commit: d3bf90c
Baseline build: 308876
Happy Coding!!

Code coverage comparison check passed!!

msfluid-bot · 2024-11-16T00:22:01Z

⯅ @fluid-example/bundle-size-tests: +245 Bytes

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	467.24 KB	467.27 KB	⯅ +35 Bytes
azureClient.js	564.01 KB	564.06 KB	⯅ +49 Bytes
connectionState.js	724 Bytes	724 Bytes	■ No change
containerRuntime.js	263.43 KB	263.45 KB	⯅ +14 Bytes
fluidFramework.js	428.51 KB	428.52 KB	⯅ +14 Bytes
loader.js	134.18 KB	134.19 KB	⯅ +14 Bytes
map.js	42.71 KB	42.71 KB	⯅ +7 Bytes
matrix.js	150.15 KB	150.16 KB	⯅ +7 Bytes
odspClient.js	529.85 KB	529.89 KB	⯅ +49 Bytes
odspDriver.js	97.88 KB	97.9 KB	⯅ +21 Bytes
odspPrefetchSnapshot.js	42.81 KB	42.83 KB	⯅ +14 Bytes
sharedString.js	166.23 KB	166.24 KB	⯅ +7 Bytes
sharedTree.js	418.97 KB	418.97 KB	⯅ +7 Bytes
Total Size	3.38 MB	3.38 MB	⯅ +245 Bytes

Baseline commit: d3bf90c

Generated by 🚫 dangerJS against 66ec5aa

jason-ha · 2024-11-17T09:13:38Z

packages/framework/presence/src/systemWorkspace.ts

+				// If attendee is connected, update their connection ID and status.
 				connectedAttendees.add(attendee);
-				if (attendee.getConnectionStatus() === SessionClientStatus.Disconnected) {
-					attendee.setConnectionId(clientConnectionId);
-				}
-				if (isNew) {
-					// If the attendee is both new and in audience (i.e. currently connected), emit an attendeeJoined event.
-					postUpdateActions.push(() => this.events.emit("attendeeJoined", attendee));
+				attendee.setConnectionId(clientConnectionId);


Doesn't ensureAttendee already do connection ID update?
Why are any of the clientConnectionIds that happen to be associated with the attendee okay to set as the current connection id.
I guess we need a test case where early connection ID is in audience, but a later one processed is not in the audience. Result should be that the early actual connected ID is the one that is should be provided by getConnectionId().

ensureAttendeemakes sure that the correct connection ID is set (updateStatus is set to false), but this function is responsible for making sure the attendees connection status is set to 'Connected'. I updated with a new function setConnected() to make this intent explicit and to avoid unnecessarily messing around with assigning correct connectionID's here.

fix test

546a2d5

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 Nov 15, 2024

WillieHabi requested review from tylerbutler and jason-ha November 15, 2024 22:08

jason-ha reviewed Nov 15, 2024

View reviewed changes

packages/framework/presence/src/systemWorkspace.ts Show resolved Hide resolved

jason-ha reviewed Nov 15, 2024

View reviewed changes

WillieHabi added 4 commits November 15, 2024 14:52

requested changes

3b9bc58

don't set connectionId in ensureAttendee

aed6004

bring back updateStatus

b3e6866

format

205368f

jason-ha reviewed Nov 15, 2024

View reviewed changes

packages/framework/presence/src/systemWorkspace.ts Outdated Show resolved Hide resolved

joining attendees

3371fb0

jason-ha reviewed Nov 15, 2024

View reviewed changes

packages/framework/presence/src/systemWorkspace.ts Outdated Show resolved Hide resolved

msfluid-bot reviewed Nov 16, 2024

View reviewed changes

jason-ha reviewed Nov 17, 2024

View reviewed changes

WillieHabi added 2 commits November 17, 2024 21:32

setConnected()

4001473

format

4df8a74

WillieHabi requested a review from jason-ha November 18, 2024 05:34

comments

833d317

WillieHabi marked this pull request as draft November 20, 2024 17:35

fixes

66ec5aa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(client-presence): disabled (failing) attendees unit tests #23113

fix(client-presence): disabled (failing) attendees unit tests #23113

WillieHabi commented Nov 15, 2024 •

edited

Loading

jason-ha Nov 15, 2024

WillieHabi Nov 15, 2024

jason-ha Nov 17, 2024

WillieHabi Nov 18, 2024

WillieHabi Nov 18, 2024 •

edited

Loading

jason-ha Nov 15, 2024

WillieHabi Nov 15, 2024 •

edited

Loading

jason-ha Nov 17, 2024

WillieHabi Nov 18, 2024

msfluid-bot left a comment •

edited

Loading

msfluid-bot commented Nov 16, 2024 •

edited

Loading

jason-ha Nov 17, 2024

WillieHabi Nov 18, 2024 •

edited

Loading

fix(client-presence): disabled (failing) attendees unit tests #23113

Are you sure you want to change the base?

fix(client-presence): disabled (failing) attendees unit tests #23113

Conversation

WillieHabi commented Nov 15, 2024 • edited Loading

Description

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 Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillieHabi Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msfluid-bot left a comment • edited Loading

Choose a reason for hiding this comment

Code Coverage Summary

Code coverage comparison check passed!!

msfluid-bot commented Nov 16, 2024 • edited Loading

Choose a reason for hiding this comment

WillieHabi Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

WillieHabi commented Nov 15, 2024 •

edited

Loading

WillieHabi Nov 18, 2024 •

edited

Loading

WillieHabi Nov 15, 2024 •

edited

Loading

msfluid-bot left a comment •

edited

Loading

msfluid-bot commented Nov 16, 2024 •

edited

Loading

WillieHabi Nov 18, 2024 •

edited

Loading