-
Notifications
You must be signed in to change notification settings - Fork 73
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
Allow for returning roomId in onAliasQuery #288
Conversation
expect(clients["bot"].createRoom).not.toHaveBeenCalled(); | ||
|
||
const room = await bridge.getRoomStore().getMatrixRoom("!abc123:bar"); | ||
expect(room).toBeDefined(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bottom part to assert the room
is just taken from the tests below but I am pretty confused about it testing anything.
If you actually debug this, room
is null
and the test seems to support both cases so that is why it passes. Seems to be a problem with all of the tests here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like you need to await onAliasQuery
to ensure that the promises all run here? This might be a bug with the other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With:
bridgeCtrl.onAliasQuery.and.returnValue({ roomId: "!abc123:bar" });
await bridge.run(101, {}, appService);
await appService.onAliasQuery("#foo:bar");
expect(clients["bot"].createRoom).not.toHaveBeenCalled();
const room = await bridge.getRoomStore().getMatrixRoom("!abc123:bar");
expect(room).toBeDefined();
expect(room.getId()).toEqual("!abc123:bar");
it seems to pass :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spotting that! Just copy pasting everything from the other tests. We should fix those other tests up too in another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #289 to fix the other tests
939a3ce
to
69e1b6c
Compare
@@ -187,6 +187,22 @@ describe("Bridge", function() { | |||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am seeing linting and test errors for something I didn't change. Am I doing something wrong?
src/components/client-factory.ts:128:62 - error TS2731: Implicit conversion of a 'symbol' to a 'string' will fail at runtime. Consider wrapping this expression in 'String(...)'.
--
|
| 128 const loggerInstance = logging.get(`js-sdk:${loggerName}`);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is #286
69e1b6c
to
53162c2
Compare
expect(clients["bot"].createRoom).not.toHaveBeenCalled(); | ||
|
||
const room = await bridge.getRoomStore().getMatrixRoom("!abc123:bar"); | ||
expect(room).toBeDefined(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like you need to await onAliasQuery
to ensure that the promises all run here? This might be a bug with the other tests.
spec/integ/bridge.spec.js
Outdated
@@ -187,6 +187,22 @@ describe("Bridge", function() { | |||
}); | |||
}); | |||
|
|||
it("should not create a room if roomId is returned from the function but should still store it", | |||
async function(done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need done
here. Some tests use it because they were written long before async
was a thing.
spec/integ/bridge.spec.js
Outdated
|
||
const room = await bridge.getRoomStore().getMatrixRoom("!abc123:bar"); | ||
expect(room).toBeDefined(); | ||
if (!room) { done(); return; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned about this, if you await onAliasQuery you shouldn't need this?
expect(clients["bot"].createRoom).not.toHaveBeenCalled(); | ||
|
||
const room = await bridge.getRoomStore().getMatrixRoom("!abc123:bar"); | ||
expect(room).toBeDefined(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With:
bridgeCtrl.onAliasQuery.and.returnValue({ roomId: "!abc123:bar" });
await bridge.run(101, {}, appService);
await appService.onAliasQuery("#foo:bar");
expect(clients["bot"].createRoom).not.toHaveBeenCalled();
const room = await bridge.getRoomStore().getMatrixRoom("!abc123:bar");
expect(room).toBeDefined();
expect(room.getId()).toEqual("!abc123:bar");
it seems to pass :)
Instead of needing to return `creationOpts`, you can now create the room yourself and return the new `roomId` you made. MR for context where we want this feature: https://gitlab.com/gitterHQ/webapp/-/merge_requests/2088
53162c2
to
583c6dd
Compare
Found while working on #288 (comment)
Add #286 to my PR so the CI passes
Found while working on #288 (comment)
Found while working on #288 (comment)
Allow for returning
roomId
inonAliasQuery
.Instead of needing to return
creationOpts
, you can now create the room yourself and return the newroomId
you made.MR for context where we want this feature: https://gitlab.com/gitterHQ/webapp/-/merge_requests/2088
As discussed in https://matrix.to/#/!ViClwbclMrgQwyPuvH:matrix.org/$VgYNMvbNXI8wjhwD5y9kI_vuQbgWgfF1-akf2VnAaNQ?via=half-shot.uk&via=matrix.org&via=tchncs.de
Dev notes