test(presence): Add snapshot tests for NotificationsManager #23124
Conversation
github-actions
bot
added
area: framework
⯅ @fluid-example/bundle-size-tests: +245 Bytes
Baseline commit: dadfe49 |
| "testEvents": { | ||
| "sessionId-2": { | ||
| "rev": 0, | ||
| "timestamp": 0, |
There was a problem hiding this comment.
Notifications have no timestamp. Not sure that is the way it should be.
There was a problem hiding this comment.
I'm glad you mentioned this. During debugging batching issues, I ended up running this both with and without the timestamp. It seems odd that it's not there, though I suppose we would never consider it trustworthy on a different client, too, and for the value types we have rev to address ordering. But I'm inclined to add it (separate change).
| // We are configuring the runtime to be in a connected state, so ensure it looks connected | ||
| runtime.connected = true; |
There was a problem hiding this comment.
shouldn't be needed - prepareConnectedPresence should handle that
| // Disable submitting signals with a dummy function. This ensures we don't capture signals from | ||
| // test setup, like the prepareConnectedPresence call. | ||
| const submitSignalOriginal = runtime.submitSignal; | ||
| runtime.submitSignal = () => {}; | ||
|
|
||
| // Set up the presence connection | ||
| presence = prepareConnectedPresence(runtime, "sessionId-2", "client2", clock, logger); | ||
|
|
||
| // Restore the submiSignal function | ||
| runtime.submitSignal = submitSignalOriginal; |
There was a problem hiding this comment.
Seems like the other solution would be to set the snapshot version of submitSignal just at the end. Don't need a special class with any overrides.
We should look at how to share test setup across mocha and vitest if we are going to have them both.
| // Verify first signal set the newId to 77 | ||
| let signal = runtime.submittedSignals[0]; | ||
| expect( | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any | ||
| (signal?.[1] as any).data["n:name:testNotificationWorkspace"].testEvents[ | ||
| "sessionId-2" | ||
| ].value.args, | ||
| ).toEqual([77]); | ||
|
|
||
| // Verify first signal set the newId to 88 | ||
| signal = runtime.submittedSignals[1]; | ||
| expect( | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any | ||
| (signal?.[1] as any).data["n:name:testNotificationWorkspace"].testEvents[ | ||
| "sessionId-2" | ||
| ].value.args, | ||
| ).toEqual([88]); |
There was a problem hiding this comment.
Philosophically:
Should we have / bother with these checks of signal content when we have snapshots? (I guess probably yes and here we are basically saying these parts of the snapshot should be harder to change accidentally.)
But if we have these, how much do we care about the other nitty gritty? Are these (and maybe few others) the only value checks that should be in place? (Big snapshots are a concern for me in that when you check everything you are over-validating and make it hard to change tangential aspects. There will be a lot of noise from such tangential changes, and it can be easy to miss that something important was changed that shouldn't have been.)
NotificationsManager or any Value Manager isn't responsible for the signal really. What matters is that others receive update in the right form with the right data. The signal checks done elsewhere were because that piece (presenceDataStoreManager.ts) is the component that does the signaling. I do think that we should have some higher-order unit integration tests that check full signals, but I am not thinking that should be the most common. Perhaps we can build support for:
- checking that a signal has certain parts (signalsExpected array could be an object or a function array - when function we call it with the current signal content to be verified (pieces are verified)) and/or
- signal generated by A and processed by B yields the expected update/notification event to caller.
Copilot
AI
left a comment
Copilot
AI
left a comment
There was a problem hiding this comment.
Copilot reviewed 4 out of 7 changed files in this pull request and generated 2 suggestions.
Files not reviewed (3)
- packages/framework/presence/package.json: Language not supported
- packages/framework/presence/src/test/snapshots/notificationsManager.vitest.js.snap: Language not supported
- packages/framework/presence/src/test/snapshots/notificationsManager.vitest.ts.snap: Language not supported
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
packages/framework/presence/src/test/notificationsManager.vitest.ts
Outdated
Show resolved
Hide resolved
tylerbutler
force-pushed
the
presence-notifications-shapshots
branch
from
bfd4912
to
947dd36
Compare
Adds a snapshot test for notifications manager that validates the following: