From e5f06df3f7ae637f677f8e227039411a63faac9c Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 24 Mar 2023 14:39:24 -0500 Subject: [PATCH] Better error handling in jump to date (#10405) - Friendly error messages with details - Add a way to submit debug logs for actual errors (non-networking errors) - Don't jump someone back to a room they already navigated away from. Fixes bug mentioned in https://github.com/vector-im/element-web/issues/21263#issuecomment-1056809714 --- src/Modal.tsx | 11 +- .../views/messages/DateSeparator.tsx | 147 ++++++++++-- src/i18n/strings/en_EN.json | 9 +- .../structures/auth/ForgotPassword-test.tsx | 38 +-- .../views/messages/DateSeparator-test.tsx | 220 ++++++++++++++++-- .../__snapshots__/DateSeparator-test.tsx.snap | 1 + .../views/right_panel/UserInfo-test.tsx | 6 +- .../views/rooms/MessageComposer-test.tsx | 15 +- .../tabs/user/SessionManagerTab-test.tsx | 5 +- test/test-utils/utilities.ts | 40 ++++ .../models/VoiceBroadcastPlayback-test.tsx | 15 +- 11 files changed, 424 insertions(+), 83 deletions(-) diff --git a/src/Modal.tsx b/src/Modal.tsx index b3887795b2..c92741cfc6 100644 --- a/src/Modal.tsx +++ b/src/Modal.tsx @@ -149,13 +149,18 @@ export class ModalManager extends TypedEventEmitter(Promise.resolve(Element), props, className); } - public closeCurrentModal(reason: string): void { + /** + * @param reason either "backgroundClick" or undefined + * @return whether a modal was closed + */ + public closeCurrentModal(reason?: string): boolean { const modal = this.getCurrentModal(); if (!modal) { - return; + return false; } modal.closeReason = reason; modal.close(); + return true; } private buildModal( @@ -346,6 +351,8 @@ export class ModalManager extends TypedEventEmitter { + // TODO: We should figure out how to remove this weird sleep. It also makes testing harder + // // await next tick because sometimes ReactDOM can race with itself and cause the modal to wrongly stick around await sleep(0); diff --git a/src/components/views/messages/DateSeparator.tsx b/src/components/views/messages/DateSeparator.tsx index 62c66b7bfb..7f2b5afc88 100644 --- a/src/components/views/messages/DateSeparator.tsx +++ b/src/components/views/messages/DateSeparator.tsx @@ -18,16 +18,19 @@ limitations under the License. import React from "react"; import { Direction } from "matrix-js-sdk/src/models/event-timeline"; import { logger } from "matrix-js-sdk/src/logger"; +import { ConnectionError, MatrixError, HTTPError } from "matrix-js-sdk/src/http-api"; import { _t } from "../../../languageHandler"; -import { formatFullDateNoTime } from "../../../DateUtils"; +import { formatFullDateNoDay, formatFullDateNoTime } from "../../../DateUtils"; import { MatrixClientPeg } from "../../../MatrixClientPeg"; -import dis from "../../../dispatcher/dispatcher"; +import dispatcher from "../../../dispatcher/dispatcher"; import { Action } from "../../../dispatcher/actions"; import SettingsStore from "../../../settings/SettingsStore"; import { UIFeature } from "../../../settings/UIFeature"; import Modal from "../../../Modal"; import ErrorDialog from "../dialogs/ErrorDialog"; +import BugReportDialog from "../dialogs/BugReportDialog"; +import AccessibleButton, { ButtonEvent } from "../elements/AccessibleButton"; import { contextMenuBelow } from "../rooms/RoomTile"; import { ContextMenuTooltipButton } from "../../structures/ContextMenu"; import IconizedContextMenu, { @@ -36,6 +39,7 @@ import IconizedContextMenu, { } from "../context_menus/IconizedContextMenu"; import JumpToDatePicker from "./JumpToDatePicker"; import { ViewRoomPayload } from "../../../dispatcher/payloads/ViewRoomPayload"; +import { SdkContextClass } from "../../../contexts/SDKContext"; function getDaysArray(): string[] { return [_t("Sunday"), _t("Monday"), _t("Tuesday"), _t("Wednesday"), _t("Thursday"), _t("Friday"), _t("Saturday")]; @@ -76,7 +80,7 @@ export default class DateSeparator extends React.Component { if (this.settingWatcherRef) SettingsStore.unwatchSetting(this.settingWatcherRef); } - private onContextMenuOpenClick = (e: React.MouseEvent): void => { + private onContextMenuOpenClick = (e: ButtonEvent): void => { e.preventDefault(); e.stopPropagation(); const target = e.target as HTMLButtonElement; @@ -118,12 +122,12 @@ export default class DateSeparator extends React.Component { private pickDate = async (inputTimestamp: number | string | Date): Promise => { const unixTimestamp = new Date(inputTimestamp).getTime(); + const roomIdForJumpRequest = this.props.roomId; - const cli = MatrixClientPeg.get(); try { - const roomId = this.props.roomId; + const cli = MatrixClientPeg.get(); const { event_id: eventId, origin_server_ts: originServerTs } = await cli.timestampToEvent( - roomId, + roomIdForJumpRequest, unixTimestamp, Direction.Forward, ); @@ -132,28 +136,113 @@ export default class DateSeparator extends React.Component { `found ${eventId} (${originServerTs}) for timestamp=${unixTimestamp} (looking forward)`, ); - dis.dispatch({ - action: Action.ViewRoom, - event_id: eventId, - highlighted: true, - room_id: roomId, - metricsTrigger: undefined, // room doesn't change - }); - } catch (e) { - const code = e.errcode || e.statusCode; - // only show the dialog if failing for something other than a network error - // (e.g. no errcode or statusCode) as in that case the redactions end up in the - // detached queue and we show the room status bar to allow retry - if (typeof code !== "undefined") { - // display error message stating you couldn't delete this. + // Only try to navigate to the room if the user is still viewing the same + // room. We don't want to jump someone back to a room after a slow request + // if they've already navigated away to another room. + const currentRoomId = SdkContextClass.instance.roomViewStore.getRoomId(); + if (currentRoomId === roomIdForJumpRequest) { + dispatcher.dispatch({ + action: Action.ViewRoom, + event_id: eventId, + highlighted: true, + room_id: roomIdForJumpRequest, + metricsTrigger: undefined, // room doesn't change + }); + } else { + logger.debug( + `No longer navigating to date in room (jump to date) because the user already switched ` + + `to another room: currentRoomId=${currentRoomId}, roomIdForJumpRequest=${roomIdForJumpRequest}`, + ); + } + } catch (err) { + logger.error( + `Error occured while trying to find event in ${roomIdForJumpRequest} ` + + `at timestamp=${unixTimestamp}:`, + err, + ); + + // Only display an error if the user is still viewing the same room. We + // don't want to worry someone about an error in a room they no longer care + // about after a slow request if they've already navigated away to another + // room. + const currentRoomId = SdkContextClass.instance.roomViewStore.getRoomId(); + if (currentRoomId === roomIdForJumpRequest) { + let friendlyErrorMessage = "An error occured while trying to find and jump to the given date."; + let submitDebugLogsContent: JSX.Element = <>; + if (err instanceof ConnectionError) { + friendlyErrorMessage = _t( + "A network error occurred while trying to find and jump to the given date. " + + "Your homeserver might be down or there was just a temporary problem with " + + "your internet connection. Please try again. If this continues, please " + + "contact your homeserver administrator.", + ); + } else if (err instanceof MatrixError) { + if (err?.errcode === "M_NOT_FOUND") { + friendlyErrorMessage = _t( + "We were unable to find an event looking forwards from %(dateString)s. " + + "Try choosing an earlier date.", + { dateString: formatFullDateNoDay(new Date(unixTimestamp)) }, + ); + } else { + friendlyErrorMessage = _t("Server returned %(statusCode)s with error code %(errorCode)s", { + statusCode: err?.httpStatus || _t("unknown status code"), + errorCode: err?.errcode || _t("unavailable"), + }); + } + } else if (err instanceof HTTPError) { + friendlyErrorMessage = err.message; + } else { + // We only give the option to submit logs for actual errors, not network problems. + submitDebugLogsContent = ( +

+ {_t( + "Please submit debug logs to help us " + + "track down the problem.", + {}, + { + debugLogsLink: (sub) => ( + ` which we + // can't nest within a `

` here so update + // this to a be a inline anchor element. + element="a" + kind="link" + onClick={() => this.onBugReport(err instanceof Error ? err : undefined)} + data-testid="jump-to-date-error-submit-debug-logs-button" + > + {sub} + + ), + }, + )} +

+ ); + } + Modal.createDialog(ErrorDialog, { - title: _t("Error"), - description: _t("Unable to find event at that date. (%(code)s)", { code }), + title: _t("Unable to find event at that date"), + description: ( +
+

{friendlyErrorMessage}

+ {submitDebugLogsContent} +
+ {_t("Error details")} +

{String(err)}

+
+
+ ), }); } } }; + private onBugReport = (err?: Error): void => { + Modal.createDialog(BugReportDialog, { + error: err, + initialText: "Error occured while using jump to date #jump-to-date", + }); + }; + private onLastWeekClicked = (): void => { const date = new Date(); date.setDate(date.getDate() - 7); @@ -189,11 +278,20 @@ export default class DateSeparator extends React.Component { onFinished={this.onContextMenuCloseClick} > - - + + @@ -207,6 +305,7 @@ export default class DateSeparator extends React.Component { return ( debug logs to help us track down the problem.": "Please submit debug logs to help us track down the problem.", + "Unable to find event at that date": "Unable to find event at that date", + "Error details": "Error details", "Last week": "Last week", "Last month": "Last month", "The beginning of the room": "The beginning of the room", diff --git a/test/components/structures/auth/ForgotPassword-test.tsx b/test/components/structures/auth/ForgotPassword-test.tsx index 5a8dbc2359..c193342a41 100644 --- a/test/components/structures/auth/ForgotPassword-test.tsx +++ b/test/components/structures/auth/ForgotPassword-test.tsx @@ -22,8 +22,13 @@ import { MatrixClient, createClient } from "matrix-js-sdk/src/matrix"; import ForgotPassword from "../../../../src/components/structures/auth/ForgotPassword"; import { ValidatedServerConfig } from "../../../../src/utils/ValidatedServerConfig"; -import { filterConsole, flushPromisesWithFakeTimers, stubClient } from "../../../test-utils"; -import Modal from "../../../../src/Modal"; +import { + clearAllModals, + filterConsole, + flushPromisesWithFakeTimers, + stubClient, + waitEnoughCyclesForModal, +} from "../../../test-utils"; import AutoDiscoveryUtils from "../../../../src/utils/AutoDiscoveryUtils"; jest.mock("matrix-js-sdk/src/matrix", () => ({ @@ -55,11 +60,6 @@ describe("", () => { }); }; - const waitForDialog = async (): Promise => { - await flushPromisesWithFakeTimers(); - await flushPromisesWithFakeTimers(); - }; - const itShouldCloseTheDialogAndShowThePasswordInput = (): void => { it("should close the dialog and show the password input", () => { expect(screen.queryByText("Verify your email to continue")).not.toBeInTheDocument(); @@ -88,9 +88,9 @@ describe("", () => { jest.spyOn(AutoDiscoveryUtils, "authComponentStateForError"); }); - afterEach(() => { + afterEach(async () => { // clean up modals - Modal.closeCurrentModal("force"); + await clearAllModals(); }); beforeAll(() => { @@ -322,7 +322,9 @@ describe("", () => { describe("and submitting it", () => { beforeEach(async () => { await click(screen.getByText("Reset password")); - await waitForDialog(); + await waitEnoughCyclesForModal({ + useFakeTimers: true, + }); }); it("should send the new password and show the click validation link dialog", () => { @@ -350,7 +352,9 @@ describe("", () => { await act(async () => { await userEvent.click(screen.getByTestId("dialog-background"), { delay: null }); }); - await waitForDialog(); + await waitEnoughCyclesForModal({ + useFakeTimers: true, + }); }); itShouldCloseTheDialogAndShowThePasswordInput(); @@ -359,7 +363,9 @@ describe("", () => { describe("and dismissing the dialog", () => { beforeEach(async () => { await click(screen.getByLabelText("Close dialog")); - await waitForDialog(); + await waitEnoughCyclesForModal({ + useFakeTimers: true, + }); }); itShouldCloseTheDialogAndShowThePasswordInput(); @@ -368,7 +374,9 @@ describe("", () => { describe("and clicking »Re-enter email address«", () => { beforeEach(async () => { await click(screen.getByText("Re-enter email address")); - await waitForDialog(); + await waitEnoughCyclesForModal({ + useFakeTimers: true, + }); }); it("should close the dialog and go back to the email input", () => { @@ -400,7 +408,9 @@ describe("", () => { beforeEach(async () => { await click(screen.getByText("Sign out of all devices")); await click(screen.getByText("Reset password")); - await waitForDialog(); + await waitEnoughCyclesForModal({ + useFakeTimers: true, + }); }); it("should show the sign out warning dialog", async () => { diff --git a/test/components/views/messages/DateSeparator-test.tsx b/test/components/views/messages/DateSeparator-test.tsx index e3001faa86..ff00cf44e9 100644 --- a/test/components/views/messages/DateSeparator-test.tsx +++ b/test/components/views/messages/DateSeparator-test.tsx @@ -16,13 +16,24 @@ limitations under the License. import React from "react"; import { mocked } from "jest-mock"; -import { render } from "@testing-library/react"; +import { fireEvent, render, screen } from "@testing-library/react"; +import { TimestampToEventResponse } from "matrix-js-sdk/src/client"; +import { ConnectionError, HTTPError, MatrixError } from "matrix-js-sdk/src/http-api"; +import dispatcher from "../../../../src/dispatcher/dispatcher"; +import { Action } from "../../../../src/dispatcher/actions"; +import { ViewRoomPayload } from "../../../../src/dispatcher/payloads/ViewRoomPayload"; +import { SdkContextClass } from "../../../../src/contexts/SDKContext"; import { formatFullDateNoTime } from "../../../../src/DateUtils"; import SettingsStore from "../../../../src/settings/SettingsStore"; import { UIFeature } from "../../../../src/settings/UIFeature"; import MatrixClientContext from "../../../../src/contexts/MatrixClientContext"; -import { getMockClientWithEventEmitter } from "../../../test-utils"; +import { + clearAllModals, + flushPromisesWithFakeTimers, + getMockClientWithEventEmitter, + waitEnoughCyclesForModal, +} from "../../../test-utils"; import DateSeparator from "../../../../src/components/views/messages/DateSeparator"; jest.mock("../../../../src/settings/SettingsStore"); @@ -31,21 +42,16 @@ describe("DateSeparator", () => { const HOUR_MS = 3600000; const DAY_MS = HOUR_MS * 24; // Friday Dec 17 2021, 9:09am - const now = "2021-12-17T08:09:00.000Z"; - const nowMs = 1639728540000; + const nowDate = new Date("2021-12-17T08:09:00.000Z"); + const roomId = "!unused:example.org"; const defaultProps = { - ts: nowMs, - now, - roomId: "!unused:example.org", + ts: nowDate.getTime(), + roomId, }; - const RealDate = global.Date; - class MockDate extends Date { - constructor(date: string | number | Date) { - super(date || now); - } - } - const mockClient = getMockClientWithEventEmitter({}); + const mockClient = getMockClientWithEventEmitter({ + timestampToEvent: jest.fn(), + }); const getComponent = (props = {}) => render( @@ -55,11 +61,11 @@ describe("DateSeparator", () => { type TestCase = [string, number, string]; const testCases: TestCase[] = [ - ["the exact same moment", nowMs, "Today"], - ["same day as current day", nowMs - HOUR_MS, "Today"], - ["day before the current day", nowMs - HOUR_MS * 12, "Yesterday"], - ["2 days ago", nowMs - DAY_MS * 2, "Wednesday"], - ["144 hours ago", nowMs - HOUR_MS * 144, "Sat, Dec 11 2021"], + ["the exact same moment", nowDate.getTime(), "Today"], + ["same day as current day", nowDate.getTime() - HOUR_MS, "Today"], + ["day before the current day", nowDate.getTime() - HOUR_MS * 12, "Yesterday"], + ["2 days ago", nowDate.getTime() - DAY_MS * 2, "Wednesday"], + ["144 hours ago", nowDate.getTime() - HOUR_MS * 144, "Sat, Dec 11 2021"], [ "6 days ago, but less than 144h", new Date("Saturday Dec 11 2021 23:59:00 GMT+0100 (Central European Standard Time)").getTime(), @@ -68,16 +74,20 @@ describe("DateSeparator", () => { ]; beforeEach(() => { - global.Date = MockDate as unknown as DateConstructor; + // Set a consistent fake time here so the test is always consistent + jest.useFakeTimers(); + jest.setSystemTime(nowDate.getTime()); + (SettingsStore.getValue as jest.Mock) = jest.fn((arg) => { if (arg === UIFeature.TimelineEnableRelativeDates) { return true; } }); + jest.spyOn(SdkContextClass.instance.roomViewStore, "getRoomId").mockReturnValue(roomId); }); afterAll(() => { - global.Date = RealDate; + jest.useRealTimers(); }); it("renders the date separator correctly", () => { @@ -115,15 +125,183 @@ describe("DateSeparator", () => { describe("when feature_jump_to_date is enabled", () => { beforeEach(() => { + jest.clearAllMocks(); mocked(SettingsStore).getValue.mockImplementation((arg): any => { if (arg === "feature_jump_to_date") { return true; } }); + jest.spyOn(dispatcher, "dispatch").mockImplementation(() => {}); }); + + afterEach(async () => { + await clearAllModals(); + }); + it("renders the date separator correctly", () => { const { asFragment } = getComponent(); expect(asFragment()).toMatchSnapshot(); }); + + [ + { + timeDescriptor: "last week", + jumpButtonTestId: "jump-to-date-last-week", + }, + { + timeDescriptor: "last month", + jumpButtonTestId: "jump-to-date-last-month", + }, + { + timeDescriptor: "the beginning", + jumpButtonTestId: "jump-to-date-beginning", + }, + ].forEach((testCase) => { + it(`can jump to ${testCase.timeDescriptor}`, async () => { + // Render the component + getComponent(); + + // Open the jump to date context menu + fireEvent.click(screen.getByTestId("jump-to-date-separator-button")); + + // Jump to "x" + const returnedDate = new Date(); + // Just an arbitrary date before "now" + returnedDate.setDate(nowDate.getDate() - 100); + const returnedEventId = "$abc"; + mockClient.timestampToEvent.mockResolvedValue({ + event_id: returnedEventId, + origin_server_ts: String(returnedDate.getTime()), + } satisfies TimestampToEventResponse); + const jumpToXButton = await screen.findByTestId(testCase.jumpButtonTestId); + fireEvent.click(jumpToXButton); + + // Flush out the dispatcher which uses `window.setTimeout(...)` since we're + // using `jest.useFakeTimers()` in these tests. + await flushPromisesWithFakeTimers(); + + // Ensure that we're jumping to the event at the given date + expect(dispatcher.dispatch).toHaveBeenCalledWith({ + action: Action.ViewRoom, + event_id: returnedEventId, + highlighted: true, + room_id: roomId, + metricsTrigger: undefined, + } satisfies ViewRoomPayload); + }); + }); + + it("should not jump to date if we already switched to another room", async () => { + // Render the component + getComponent(); + + // Open the jump to date context menu + fireEvent.click(screen.getByTestId("jump-to-date-separator-button")); + + // Mimic the outcome of switching rooms while waiting for the jump to date + // request to finish. Imagine that we started jumping to "last week", the + // network request is taking a while, so we got bored, switched rooms; we + // shouldn't jump back to the previous room after the network request + // happens to finish later. + jest.spyOn(SdkContextClass.instance.roomViewStore, "getRoomId").mockReturnValue("!some-other-room"); + + // Jump to "last week" + mockClient.timestampToEvent.mockResolvedValue({ + event_id: "$abc", + origin_server_ts: "0", + }); + const jumpToLastWeekButton = await screen.findByTestId("jump-to-date-last-week"); + fireEvent.click(jumpToLastWeekButton); + + // Flush out the dispatcher which uses `window.setTimeout(...)` since we're + // using `jest.useFakeTimers()` in these tests. + await flushPromisesWithFakeTimers(); + + // We should not see any room switching going on (`Action.ViewRoom`) + expect(dispatcher.dispatch).not.toHaveBeenCalled(); + }); + + it("should not show jump to date error if we already switched to another room", async () => { + // Render the component + getComponent(); + + // Open the jump to date context menu + fireEvent.click(screen.getByTestId("jump-to-date-separator-button")); + + // Mimic the outcome of switching rooms while waiting for the jump to date + // request to finish. Imagine that we started jumping to "last week", the + // network request is taking a while, so we got bored, switched rooms; we + // shouldn't jump back to the previous room after the network request + // happens to finish later. + jest.spyOn(SdkContextClass.instance.roomViewStore, "getRoomId").mockReturnValue("!some-other-room"); + + // Try to jump to "last week" but we want an error to occur and ensure that + // we don't show an error dialog for it since we already switched away to + // another room and don't care about the outcome here anymore. + mockClient.timestampToEvent.mockRejectedValue(new Error("Fake error in test")); + const jumpToLastWeekButton = await screen.findByTestId("jump-to-date-last-week"); + fireEvent.click(jumpToLastWeekButton); + + // Wait the necessary time in order to see if any modal will appear + await waitEnoughCyclesForModal({ + useFakeTimers: true, + }); + + // We should not see any error modal dialog + // + // We have to use `queryBy` so that it can return `null` for something that does not exist. + expect(screen.queryByTestId("jump-to-date-error-content")).not.toBeInTheDocument(); + }); + + it("should show error dialog with submit debug logs option when non-networking error occurs", async () => { + // Render the component + getComponent(); + + // Open the jump to date context menu + fireEvent.click(screen.getByTestId("jump-to-date-separator-button")); + + // Try to jump to "last week" but we want a non-network error to occur so it + // shows the "Submit debug logs" UI + mockClient.timestampToEvent.mockRejectedValue(new Error("Fake error in test")); + const jumpToLastWeekButton = await screen.findByTestId("jump-to-date-last-week"); + fireEvent.click(jumpToLastWeekButton); + + // Expect error to be shown. We have to wait for the UI to transition. + expect(await screen.findByTestId("jump-to-date-error-content")).toBeInTheDocument(); + + // Expect an option to submit debug logs to be shown when a non-network error occurs + expect(await screen.findByTestId("jump-to-date-error-submit-debug-logs-button")).toBeInTheDocument(); + }); + + [ + new ConnectionError("Fake connection error in test"), + new HTTPError("Fake http error in test", 418), + new MatrixError( + { errcode: "M_FAKE_ERROR_CODE", error: "Some fake error occured" }, + 518, + "https://fake-url/", + ), + ].forEach((fakeError) => { + it(`should show error dialog without submit debug logs option when networking error (${fakeError.name}) occurs`, async () => { + // Render the component + getComponent(); + + // Open the jump to date context menu + fireEvent.click(screen.getByTestId("jump-to-date-separator-button")); + + // Try to jump to "last week" but we want a network error to occur + mockClient.timestampToEvent.mockRejectedValue(fakeError); + const jumpToLastWeekButton = await screen.findByTestId("jump-to-date-last-week"); + fireEvent.click(jumpToLastWeekButton); + + // Expect error to be shown. We have to wait for the UI to transition. + expect(await screen.findByTestId("jump-to-date-error-content")).toBeInTheDocument(); + + // The submit debug logs option should *NOT* be shown for network errors. + // + // We have to use `queryBy` so that it can return `null` for something that does not exist. + expect(screen.queryByTestId("jump-to-date-error-submit-debug-logs-button")).not.toBeInTheDocument(); + }); + }); }); }); diff --git a/test/components/views/messages/__snapshots__/DateSeparator-test.tsx.snap b/test/components/views/messages/__snapshots__/DateSeparator-test.tsx.snap index d0af14f509..16151fc000 100644 --- a/test/components/views/messages/__snapshots__/DateSeparator-test.tsx.snap +++ b/test/components/views/messages/__snapshots__/DateSeparator-test.tsx.snap @@ -44,6 +44,7 @@ exports[`DateSeparator when feature_jump_to_date is enabled renders the date sep aria-haspopup="true" aria-label="Jump to date" class="mx_AccessibleButton mx_DateSeparator_jumpToDateMenu mx_DateSeparator_dateContent" + data-testid="jump-to-date-separator-button" role="button" tabindex="0" > diff --git a/test/components/views/right_panel/UserInfo-test.tsx b/test/components/views/right_panel/UserInfo-test.tsx index 140cd14ffe..7ca7daa8f2 100644 --- a/test/components/views/right_panel/UserInfo-test.tsx +++ b/test/components/views/right_panel/UserInfo-test.tsx @@ -45,7 +45,7 @@ import * as mockVerification from "../../../../src/verification"; import Modal from "../../../../src/Modal"; import { E2EStatus } from "../../../../src/utils/ShieldUtils"; import { DirectoryMember, startDmOnFirstMessage } from "../../../../src/utils/direct-messages"; -import { flushPromises } from "../../../test-utils"; +import { clearAllModals, flushPromises } from "../../../test-utils"; jest.mock("../../../../src/utils/direct-messages", () => ({ ...jest.requireActual("../../../../src/utils/direct-messages"), @@ -417,7 +417,9 @@ describe("", () => { mockClient.setIgnoredUsers.mockClear(); }); - afterEach(() => Modal.closeCurrentModal("End of test")); + afterEach(async () => { + await clearAllModals(); + }); afterAll(() => { inviteSpy.mockRestore(); diff --git a/test/components/views/rooms/MessageComposer-test.tsx b/test/components/views/rooms/MessageComposer-test.tsx index 3ffe68424d..d2a6e9c12e 100644 --- a/test/components/views/rooms/MessageComposer-test.tsx +++ b/test/components/views/rooms/MessageComposer-test.tsx @@ -21,6 +21,7 @@ import { act, render, screen } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { + clearAllModals, createTestClient, filterConsole, flushPromises, @@ -28,6 +29,7 @@ import { mkStubRoom, mockPlatformPeg, stubClient, + waitEnoughCyclesForModal, } from "../../../test-utils"; import MessageComposer from "../../../../src/components/views/rooms/MessageComposer"; import MatrixClientContext from "../../../../src/contexts/MatrixClientContext"; @@ -48,7 +50,6 @@ import { Action } from "../../../../src/dispatcher/actions"; import { VoiceBroadcastInfoState, VoiceBroadcastRecording } from "../../../../src/voice-broadcast"; import { mkVoiceBroadcastInfoStateEvent } from "../../../voice-broadcast/utils/test-utils"; import { SdkContextClass } from "../../../../src/contexts/SDKContext"; -import Modal from "../../../../src/Modal"; jest.mock("../../../../src/components/views/rooms/wysiwyg_composer", () => ({ SendWysiwygComposer: jest.fn().mockImplementation(() =>
), @@ -77,15 +78,9 @@ const setCurrentBroadcastRecording = (room: Room, state: VoiceBroadcastInfoState SdkContextClass.instance.voiceBroadcastRecordingsStore.setCurrent(recording); }; -const waitForModal = async (): Promise => { - await flushPromises(); - await flushPromises(); -}; - const shouldClearModal = async (): Promise => { afterEach(async () => { - Modal.closeCurrentModal("force"); - await waitForModal(); + await clearAllModals(); }); }; @@ -434,7 +429,7 @@ describe("MessageComposer", () => { setCurrentBroadcastRecording(room, VoiceBroadcastInfoState.Started); wrapAndRender({ room }); await startVoiceMessage(); - await waitForModal(); + await waitEnoughCyclesForModal(); }); shouldClearModal(); @@ -450,7 +445,7 @@ describe("MessageComposer", () => { setCurrentBroadcastRecording(room, VoiceBroadcastInfoState.Stopped); wrapAndRender({ room }); await startVoiceMessage(); - await waitForModal(); + await waitEnoughCyclesForModal(); }); shouldClearModal(); diff --git a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx index 8358922527..fcfca9e303 100644 --- a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx +++ b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx @@ -31,6 +31,7 @@ import { IAuthData, } from "matrix-js-sdk/src/matrix"; +import { clearAllModals } from "../../../../../test-utils"; import SessionManagerTab from "../../../../../../src/components/views/settings/tabs/user/SessionManagerTab"; import MatrixClientContext from "../../../../../../src/contexts/MatrixClientContext"; import { @@ -161,7 +162,7 @@ describe("", () => { await flushPromises(); }; - beforeEach(() => { + beforeEach(async () => { jest.clearAllMocks(); jest.spyOn(logger, "error").mockRestore(); mockClient.getStoredDevice.mockImplementation((_userId, id) => { @@ -199,7 +200,7 @@ describe("", () => { // sometimes a verification modal is in modal state when these tests run // make sure the coast is clear - Modal.closeCurrentModal(""); + await clearAllModals(); }); it("renders spinner while devices load", () => { diff --git a/test/test-utils/utilities.ts b/test/test-utils/utilities.ts index 2549b5971e..420e5d3bca 100644 --- a/test/test-utils/utilities.ts +++ b/test/test-utils/utilities.ts @@ -19,6 +19,7 @@ import EventEmitter from "events"; import { ActionPayload } from "../../src/dispatcher/payloads"; import defaultDispatcher from "../../src/dispatcher/dispatcher"; import { DispatcherAction } from "../../src/dispatcher/actions"; +import Modal from "../../src/Modal"; export const emitPromise = (e: EventEmitter, k: string | symbol) => new Promise((r) => e.once(k, r)); @@ -174,3 +175,42 @@ export const advanceDateAndTime = (ms: number) => { jest.spyOn(global.Date, "now").mockReturnValue(Date.now() + ms); jest.advanceTimersByTime(ms); }; + +/** + * A horrible hack necessary to wait enough time to ensure any modal is shown after a + * `Modal.createDialog(...)` call. We have to contend with the Modal code which renders + * things asyncronhously and has weird sleeps which we should strive to remove. + */ +export const waitEnoughCyclesForModal = async ({ + useFakeTimers = false, +}: { + useFakeTimers?: boolean; +} = {}): Promise => { + // XXX: Maybe in the future with Jest 29.5.0+, we could use `runAllTimersAsync` instead. + const flushFunc = useFakeTimers ? flushPromisesWithFakeTimers : flushPromises; + + await flushFunc(); + await flushFunc(); + await flushFunc(); +}; + +/** + * A horrible hack necessary to make sure modals don't leak and pollute tests. + * `@testing-library/react` automatic cleanup function does not pick up the async modal + * rendering and the modals don't unmount when the component unmounts. We should strive + * to fix this. + */ +export const clearAllModals = async (): Promise => { + // Prevent modals from leaking and polluting other tests + let keepClosingModals = true; + while (keepClosingModals) { + keepClosingModals = Modal.closeCurrentModal("End of test (clean-up)"); + + // Then wait for the screen to update (probably React rerender and async/await). + // Important for tests using Jest fake timers to not get into an infinite loop + // of removing the same modal because the promises don't flush otherwise. + // + // XXX: Maybe in the future with Jest 29.5.0+, we could use `runAllTimersAsync` instead. + await flushPromisesWithFakeTimers(); + } +}; diff --git a/test/voice-broadcast/models/VoiceBroadcastPlayback-test.tsx b/test/voice-broadcast/models/VoiceBroadcastPlayback-test.tsx index 9f59ba6369..9ee165d87a 100644 --- a/test/voice-broadcast/models/VoiceBroadcastPlayback-test.tsx +++ b/test/voice-broadcast/models/VoiceBroadcastPlayback-test.tsx @@ -32,7 +32,13 @@ import { VoiceBroadcastPlaybackState, VoiceBroadcastRecording, } from "../../../src/voice-broadcast"; -import { filterConsole, flushPromises, flushPromisesWithFakeTimers, stubClient } from "../../test-utils"; +import { + filterConsole, + flushPromises, + flushPromisesWithFakeTimers, + stubClient, + waitEnoughCyclesForModal, +} from "../../test-utils"; import { createTestPlayback } from "../../test-utils/audio"; import { mkVoiceBroadcastChunkEvent, mkVoiceBroadcastInfoStateEvent } from "../utils/test-utils"; import { LazyValue } from "../../../src/utils/LazyValue"; @@ -77,11 +83,6 @@ describe("VoiceBroadcastPlayback", () => { ); }; - const waitForDialog = async () => { - await flushPromises(); - await flushPromises(); - }; - const itShouldSetTheStateTo = (state: VoiceBroadcastPlaybackState) => { it(`should set the state to ${state}`, () => { expect(playback.getState()).toBe(state); @@ -480,7 +481,7 @@ describe("VoiceBroadcastPlayback", () => { jest.spyOn(recording, "stop"); SdkContextClass.instance.voiceBroadcastRecordingsStore.setCurrent(recording); playback.start(); - await waitForDialog(); + await waitEnoughCyclesForModal(); }); it("should display a confirm modal", () => {