Refactor local transport testing and local membership initialization #3576
Conversation
toger5
added
the
PR-Developer-Experience
toger5
force-pushed
the
toger5/fix-skipped-tests
branch
from
f3633d2 to
1fd9ac9
Compare
BillCarsonFr
left a comment
BillCarsonFr
left a comment
There was a problem hiding this comment.
Great refactor 👍
Just a few comments
src/livekit/openIDSFU.ts
Outdated
| >; | ||
|
|
||
| /** | ||
| * |
There was a problem hiding this comment.
Suggestion?
Gets a bearer token from the homeserver and then use it to authenticate to the matrix RTC backend in order to get acces to the SFU.
It has built-in retry for calls to the homeserver with a backoff policy. Will bubble errors from the matrix RTC backend :(.
src/room/GroupCallErrorBoundary.tsx
Outdated
| }: ErrorPageProps): ReactElement => { | ||
| const { t } = useTranslation(); | ||
|
|
||
| logger.log("Error boundary caught:", error); |
| livekit_service_url: "http://my-oldest-member-service-url.com", | ||
| livekit_alias: "my-oldest-member-service-alias", | ||
| }; | ||
| // vi.mock("../../../widget", async (importOriginal) => ({ |
| /** | ||
| * Whether we are connected to the MatrixRTC session. | ||
| */ | ||
| export function createHomeserverConnected$( |
There was a problem hiding this comment.
I think we talked about extracting that to its own file? with some tests. Maybe next PR
| * @prop useOldestMember Whether to use the same transport as the oldest member. | ||
| * This will only update once the first oldest member appears. Will not recompute if the oldest member leaves. | ||
| * | ||
| * @throws MatrixRTCTransportMissingError | FailToGetOpenIdToken |
There was a problem hiding this comment.
👍 I wonder if we can have a linter rule that uses that information to ensure the caller handle the errors
There was a problem hiding this comment.
I searched a bit. not possible.
| transportFromStorage, | ||
| ); | ||
| return transportFromStorage; | ||
| transport = transportFromStorage; |
There was a problem hiding this comment.
Can we have a test for that? (checking that is trying local storage)
| }; | ||
| logger.log("Using LiveKit transport from config: ", transportFromConf); | ||
| return transportFromConf; | ||
| transport = transportFromConf; |
There was a problem hiding this comment.
is it also returning transport?
There was a problem hiding this comment.
this needed some update (the priority should have been the other way around) updated it.
| connectOptions$.value, | ||
| ); | ||
| }, | ||
| createPublisherFactory: (connection: Connection) => { |
There was a problem hiding this comment.
nit: maybe we can extract it as a const just before instead of inlining? WDYT
There was a problem hiding this comment.
This was consciously done inline to make clear that this is "one Package" of input and not entangled with the rest of the call view model.
So that its easier to understand why callviewmodel test just mock the while createLocalMembership$ return value.
Half-Shot
changed the title
| localTransportCanThrow$.pipe( | ||
| catchError((e: unknown) => { | ||
| let error: ElementCallError; | ||
| if (e instanceof ElementCallError) { |
There was a problem hiding this comment.
I think we have that at several place in the code, we might want some utility at some point
toger5
force-pushed
the
toger5/fix-skipped-tests
branch
from
176c26c to
e60bc9e
Compare
A test in the callViewModel uncovered that there is a much better way to split test for the call setup.
This splits the test into localTransport tests that check for the different error cases and a test for the local Membership that checks that on error we will end up in the correct state for the callViewModel. (based on this state we set
fatalError$