Skip to content
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

feat(logging): Use loglevel library for error handling #1845

Merged
merged 5 commits into from
Feb 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
"fuse.js": "7.0.0",
"lodash.isequal": "4.5.0",
"lodash.mergewith": "4.6.2",
"loglevel": "1.9.2",
"prism-react-renderer": "2.4.1",
"prismjs": "1.29.0",
"react": "18.3.1",
Expand Down
10 changes: 0 additions & 10 deletions src/components/App/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,7 @@ vi.mock("react-router", async () => {
const mockStore = configureStore([]);

describe("App", () => {
let consoleError: Console["error"];

beforeEach(() => {
consoleError = console.error;
// Even though the error boundary catches the error, there is still a
// console.error in the test output.
console.error = vi.fn();
});

afterEach(() => {
console.error = consoleError;
vi.resetAllMocks();
vi.unstubAllEnvs();
});
Expand Down
5 changes: 0 additions & 5 deletions src/components/LogIn/LogIn.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,6 @@ describe("LogIn", () => {
});

it("should remove the authentication request when clicking the authenticate button", async () => {
const consoleError = console.error;
console.error = vi.fn();

const state = rootStateFactory.build({
general: generalStateFactory.withConfig().build({
config: configFactory.build({
Expand All @@ -186,7 +183,5 @@ describe("LogIn", () => {
{ pointerEventsCheck: 0 },
);
expect(screen.queryByTestId("toast-card")).not.toBeInTheDocument();

console.error = consoleError;
});
});
38 changes: 0 additions & 38 deletions src/components/LogIn/UserPassForm/UserPassForm.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,9 @@ import { generalStateFactory } from "testing/factories/general";
import { rootStateFactory } from "testing/factories/root";
import { renderComponent } from "testing/utils";

import { Label } from "../types";

import UserPassForm from "./UserPassForm";

describe("UserPassForm", () => {
afterEach(() => {
vi.restoreAllMocks();
});

it("should log in", async () => {
// Mock the result of the thunk to be a normal action so that it can be tested
// for. This is necessary because we don't have a full store set up which
Expand Down Expand Up @@ -57,36 +51,4 @@ describe("UserPassForm", () => {
type: "connectAndStartPolling",
});
});

it("should display console error when trying to log in", async () => {
const consoleError = console.error;
console.error = vi.fn();

vi.spyOn(appThunks, "connectAndStartPolling").mockImplementation(
vi.fn().mockReturnValue({ type: "connectAndStartPolling" }),
);
vi.spyOn(dashboardStore, "useAppDispatch").mockImplementation(
vi
.fn()
.mockReturnValue((action: unknown) =>
action instanceof Object &&
"type" in action &&
action.type === "connectAndStartPolling"
? Promise.reject(
new Error("Error while dispatching connectAndStartPolling!"),
)
: null,
),
);

renderComponent(<UserPassForm />);
await userEvent.click(screen.getByRole("button"));
expect(appThunks.connectAndStartPolling).toHaveBeenCalledTimes(1);
expect(console.error).toHaveBeenCalledWith(
Label.POLLING_ERROR,
new Error("Error while dispatching connectAndStartPolling!"),
);

console.error = consoleError;
});
});
3 changes: 2 additions & 1 deletion src/components/LogIn/UserPassForm/UserPassForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { thunks as appThunks } from "store/app";
import { actions as generalActions } from "store/general";
import { getWSControllerURL } from "store/general/selectors";
import { useAppDispatch, useAppSelector } from "store/store";
import { logger } from "utils/logger";

import { Label } from "../types";

Expand Down Expand Up @@ -35,7 +36,7 @@ const UserPassForm = () => {
if (bakery) {
dispatch(appThunks.connectAndStartPolling())
.then(unwrapResult)
.catch((error) => console.error(Label.POLLING_ERROR, error));
.catch((error) => logger.error(Label.POLLING_ERROR, error));
}
}

Expand Down
25 changes: 0 additions & 25 deletions src/components/ShareCard/ShareCard.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -73,29 +73,4 @@ describe("Share Card", () => {
await userEvent.selectOptions(screen.getByRole("combobox"), "write");
expect(accessSelectChangeFn).toHaveBeenCalled();
});

it("should display console error when trying to change access", async () => {
const consoleError = console.error;
console.error = vi.fn();

const removeUserFn = vi.fn();
const accessSelectChangeFn = vi.fn(() => Promise.reject(new Error()));
render(
<ShareCard
userName="janedoe"
lastConnected="2021-06-03T16:03:15Z"
access="read"
isOwner={false}
removeUser={removeUserFn}
accessSelectChange={accessSelectChangeFn}
/>,
);
await userEvent.selectOptions(screen.getByRole("combobox"), "write");
expect(console.error).toHaveBeenCalledWith(
Label.ACCESS_CHANGE_ERROR,
new Error(),
);

console.error = consoleError;
});
});
3 changes: 2 additions & 1 deletion src/components/ShareCard/ShareCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { useEffect, useState } from "react";
import SlideDownFadeOut from "animations/SlideDownFadeOut";
import TruncatedTooltip from "components/TruncatedTooltip";
import { formatFriendlyDateToNow } from "components/utils";
import { logger } from "utils/logger";

import "./_share-card.scss";
import { Label } from "./types";
Expand Down Expand Up @@ -117,7 +118,7 @@ export default function ShareCard({
return;
})
.catch((error) =>
console.error(Label.ACCESS_CHANGE_ERROR, error),
logger.error(Label.ACCESS_CHANGE_ERROR, error),
);
}}
value={access}
Expand Down
3 changes: 0 additions & 3 deletions src/components/UserMenu/UserMenu.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import UserMenu from "./UserMenu";

describe("User Menu", () => {
let state: RootState;
const consoleError = console.error;

beforeEach(() => {
state = rootStateFactory.build({
Expand All @@ -33,12 +32,10 @@ describe("User Menu", () => {
},
}),
});
console.error = vi.fn();
});

afterEach(() => {
vi.restoreAllMocks();
console.error = consoleError;
});

it("is inactive by default", () => {
Expand Down
14 changes: 0 additions & 14 deletions src/hooks/useLocalStorage.test.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,9 @@
import { act, renderHook } from "@testing-library/react";
import { vi } from "vitest";

import useLocalStorage from "./useLocalStorage";

describe("useLocalStorage", () => {
let consoleError: Console["error"];

beforeEach(() => {
consoleError = console.error;
console.error = vi.fn();
});

afterEach(() => {
console.error = consoleError;
localStorage.clear();
});

Expand All @@ -38,10 +29,6 @@ describe("useLocalStorage", () => {
const { result } = renderHook(() =>
useLocalStorage("test-key", "init-val"),
);
expect(console.error).toHaveBeenCalledWith(
"Unable to parse local storage:",
expect.any(Error),
);
const [value] = result.current;
expect(value).toBe("init-val");
});
Expand Down Expand Up @@ -71,7 +58,6 @@ describe("useLocalStorage", () => {
act(() => {
setValue(circular as unknown as string);
});
expect(console.error).toHaveBeenCalledWith(expect.any(Error));
const [value] = result.current;
expect(value).toBe("init-val");
expect(JSON.parse(localStorage.getItem("test-key") ?? "")).toBe("init-val");
Expand Down
6 changes: 4 additions & 2 deletions src/hooks/useLocalStorage.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { useState } from "react";

import { logger } from "utils/logger";

function useLocalStorage<V>(
key: string,
initialValue: V,
Expand All @@ -12,7 +14,7 @@ function useLocalStorage<V>(
return item ? JSON.parse(item) : initialValue;
} catch (error) {
// Not shown in UI. Logged for debugging purposes.
console.error("Unable to parse local storage:", error);
logger.error("Unable to parse local storage:", error);
return initialValue;
}
});
Expand All @@ -26,7 +28,7 @@ function useLocalStorage<V>(
window.localStorage.setItem(key, stringified);
} catch (error) {
// Not shown in UI. Logged for debugging purposes.
console.error(error);
logger.error(error);
}
};

Expand Down
10 changes: 0 additions & 10 deletions src/hooks/useLogout.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,6 @@ import { renderComponent, renderWrappedHook } from "testing/utils";
import useLogout from "./useLogout";

describe("useLogout", () => {
const consoleError = console.error;

beforeEach(() => {
console.error = vi.fn();
});

afterEach(() => {
console.error = consoleError;
});

it("should logout", async () => {
vi.spyOn(appThunks, "logOut").mockImplementation(
vi.fn().mockReturnValue({ type: "logOut", catch: vi.fn() }),
Expand Down
3 changes: 2 additions & 1 deletion src/hooks/useLogout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import reactHotToast from "react-hot-toast";
import ToastCard from "components/ToastCard";
import { thunks as appThunks } from "store/app";
import { useAppDispatch } from "store/store";
import { logger } from "utils/logger";

export enum Label {
LOGOUT_ERROR = "Error when trying to logout.",
Expand All @@ -30,7 +31,7 @@ const useLogout = () => {
</>
</ToastCard>
));
console.error(Label.LOGOUT_ERROR, error);
logger.error(Label.LOGOUT_ERROR, error);
});
};
};
Expand Down
4 changes: 3 additions & 1 deletion src/hooks/useQueryParams.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import { useCallback } from "react";
import type { NavigateOptions } from "react-router";
import { useSearchParams } from "react-router";

import { logger } from "utils/logger";

export type SetParams<P> = (
params?: Partial<P> | null,
options?: NavigateOptions,
Expand Down Expand Up @@ -42,7 +44,7 @@ export const useQueryParams = <P extends QueryParams>(
const sanitizedKey = DOMPurify.sanitize(key);
if (key !== sanitizedKey) {
searchParams.delete(key);
console.log(
logger.log(
`Query param key "${key}" has been changed to "${sanitizedKey}"` +
" in order to prevent potential XSS Attacks.",
);
Expand Down
45 changes: 0 additions & 45 deletions src/index.test.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { screen, waitFor } from "@testing-library/dom";
import type { UnknownAction } from "redux";
import { vi } from "vitest";

import * as storeModule from "store";
Expand Down Expand Up @@ -37,7 +36,6 @@ const appVersion = packageJSON.version;

describe("renderApp", () => {
let rootNode: HTMLElement;
const consoleError = console.error;
const windowLocation = window.location;

beforeEach(() => {
Expand All @@ -46,8 +44,6 @@ describe("renderApp", () => {
enumerable: true,
value: new URL(window.location.href),
});
// Hide the config errors from the test output.
console.error = vi.fn();
rootNode = document.createElement("div");
rootNode.setAttribute("id", ROOT_ID);
document.body.appendChild(rootNode);
Expand All @@ -59,7 +55,6 @@ describe("renderApp", () => {
enumerable: true,
value: windowLocation,
});
console.error = consoleError;
if (rootNode) {
document.body.removeChild(rootNode);
}
Expand Down Expand Up @@ -250,16 +245,6 @@ describe("renderApp", () => {
});

describe("getControllerAPIEndpointErrors", () => {
const consoleError = console.error;

beforeEach(() => {
console.error = vi.fn();
});

afterEach(() => {
console.error = consoleError;
});

it("should handle secure protocol", () => {
expect(
getControllerAPIEndpointErrors("wss://example.com:80/api"),
Expand Down Expand Up @@ -305,34 +290,4 @@ describe("getControllerAPIEndpointErrors", () => {
"controllerAPIEndpoint (example.com:80/api) must be an absolute path or begin with ws:// or wss://.",
);
});

it("should show console error when dispatching connectAndStartPolling", async () => {
vi.spyOn(appThunks, "connectAndStartPolling").mockImplementation(
vi.fn().mockReturnValue({ type: "connectAndStartPolling" }),
);
vi.spyOn(storeModule.default, "dispatch").mockImplementation(
(action) =>
(action instanceof Object &&
"type" in action &&
action.type === "connectAndStartPolling"
? Promise.reject(
new Error("Error while dispatching connectAndStartPolling!"),
)
: null) as unknown as UnknownAction,
);
const config = configFactory.build({
baseControllerURL: null,
identityProviderURL: "/candid",
isJuju: true,
});
window.jujuDashboardConfig = config;
renderApp();
expect(appThunks.connectAndStartPolling).toHaveBeenCalledTimes(1);
await waitFor(() =>
expect(console.error).toHaveBeenCalledWith(
Label.POLLING_ERROR,
new Error("Error while dispatching connectAndStartPolling!"),
),
);
});
});
Loading