Conversation
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
This test failed when ran individually, same as after the clearAllMocks call Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
…ny other way Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
…ch-mock & fix typo Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
t3chguy
requested review from
AndrewFerr,
MidhunSureshR,
kaylendog and
richvdh
BillCarsonFr
left a comment
BillCarsonFr
left a comment
There was a problem hiding this comment.
Thanks for the detailed breakdown and descriptions. Makes the review easier.
Just some non-blocking comments
| fetchMock.get("path:/_matrix/client/v3/room_keys/version", testData.SIGNED_BACKUP_DATA); | ||
|
|
||
| const rustCrypto = await makeTestRustCrypto(makeMatrixHttpApi()); | ||
| await expect(rustCrypto.getKeyBackupInfo()).resolves.toStrictEqual(testData.SIGNED_BACKUP_DATA); |
There was a problem hiding this comment.
Looks like toStrictEqual is asserting more tham toMatchObject? any reason to relax the requirement?
There was a problem hiding this comment.
With vitest it was failing with Serializes to same value
|
|
||
| expect(groupCall.screenshareFeeds).toStrictEqual(newFeeds); | ||
| expect(currentFeed.dispose).toHaveBeenCalled(); | ||
| expect(newFeed.measureVolumeActivity).toHaveBeenCalledWith(true); |
There was a problem hiding this comment.
Good catch. It is legacy calling? wonder if we shouldn't create an issue to track that this test was only passing due to side effect of other test
There was a problem hiding this comment.
Looking through the code it looks like measureVolumeActivity doesn't apply to screenshare streams
| // Then it disappears from the thread and appears in the main timeline | ||
| expect(ev.threadRootId).toBeUndefined(); | ||
| expect(mainTimelineLiveEventIds(room)).toEqual([threadRoot.getId(), ev.getId()]); | ||
| expect(mainTimelineLiveEventIds(room)).toEqual([threadRoot.getId(), ev.getId(), redaction.getId()]); |
There was a problem hiding this comment.
That is strange.. Isn't it more that redaction.getId() is undefined?
like expect([1,2]).toEqual([1,2, undefined]);
Maybe these test could use things like expect.arrayContaining([threadRoot.getId(), ev.getId()]) ?
There was a problem hiding this comment.
Yes, redaction before this PR had an undefined ID which jest allows you to skip in toEqual - vitest was unhappy and I thought that given real redactions have event IDs it'd make sense to make them have IDs in test too and assert them correctly
arrayContaining wouldn't assert order
|
|
||
| it("should copy state from previous timeline", async function () { | ||
| // XXX: This test was previously not running and it is broken | ||
| it.skip("should copy state from previous timeline", async function () { |
There was a problem hiding this comment.
Out of curiosity, is there a task to investigate if this is important or not?
There was a problem hiding this comment.
Not from me, all the tests in this weird suite weren't running at all so this is preferable than removing them all to match the prior reality
andybalaam
requested review from
andybalaam
and removed request for
kaylendog and
richvdh
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
Split out from #5136
I suggest reviewing commits to get some context