Skip to content

Clean shared components test log #32759

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.

Sign up for GitHub

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

Closed
florianduros wants to merge 2 commits into from
+26 −15
Closed

Clean shared components test log #32759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
94f5ef9
chore: hide loading language log when sc tests are run
florianduros Mar 9, 2026
1f23267
chore: fix resize observer logging error
florianduros Mar 9, 2026
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
32 changes: 19 additions & 13 deletions ...es/shared-components/src/room-list/RoomListPrimaryFilters/RoomListPrimaryFilters.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,27 +68,33 @@ describe("<RoomListPrimaryFilters /> stories", () => {
} as unknown as typeof ResizeObserver;
});

function mockFiltersNotWrapping(): void {
async function mockFiltersNotWrapping(): Promise<void> {
vi.spyOn(screen.getByText("People"), "offsetLeft", "get").mockReturnValue(0);
vi.spyOn(screen.getByText("Rooms"), "offsetLeft", "get").mockReturnValue(30);
vi.spyOn(screen.getByText("Unreads"), "offsetLeft", "get").mockReturnValue(60);

const listbox = screen.getByRole("listbox", { name: "Room list filters" });
act(() => resizeCallback([{ target: listbox } as any], {} as ResizeObserver));
await act(async () => {
resizeCallback([{ target: listbox } as any], {} as ResizeObserver);
await new Promise<void>((resolve) => requestAnimationFrame(() => resolve()));
});
}

function mockUnreadWrapping(): void {
async function mockUnreadWrapping(): Promise<void> {
vi.spyOn(screen.getByText("People"), "offsetLeft", "get").mockReturnValue(0);
vi.spyOn(screen.getByText("Rooms"), "offsetLeft", "get").mockReturnValue(30);
vi.spyOn(screen.getByText("Unreads"), "offsetLeft", "get").mockReturnValue(0);

const listbox = screen.getByRole("listbox", { name: "Room list filters" });
act(() => resizeCallback([{ target: listbox } as any], {} as ResizeObserver));
await act(async () => {
resizeCallback([{ target: listbox } as any], {} as ResizeObserver);
await new Promise<void>((resolve) => requestAnimationFrame(() => resolve()));
});
}

it("should hide wrapping filters and show chevron", () => {
it("should hide wrapping filters and show chevron", async () => {
render(<NarrowContainer />);
mockUnreadWrapping();
await mockUnreadWrapping();

expect(screen.queryByRole("option", { name: "Unreads" })).toBeNull();
expect(screen.getByRole("button", { name: "Expand filter list" })).toBeInTheDocument();
Expand All @@ -97,7 +103,7 @@ describe("<RoomListPrimaryFilters /> stories", () => {
it("should expand and collapse filter list with chevron button", async () => {
const user = userEvent.setup();
render(<NarrowContainer />);
mockUnreadWrapping();
await mockUnreadWrapping();

expect(screen.queryByRole("option", { name: "Unreads" })).toBeNull();

Expand All @@ -108,9 +114,9 @@ describe("<RoomListPrimaryFilters /> stories", () => {
expect(screen.queryByRole("option", { name: "Unreads" })).toBeNull();
});

it("should move active filter to front when collapsed and wrapping", () => {
it("should move active filter to front when collapsed and wrapping", async () => {
render(<NarrowWithActiveWrappingFilter />);
mockUnreadWrapping();
await mockUnreadWrapping();

const listbox = screen.getByRole("listbox", { name: "Room list filters" });
expect(listbox.children[0]).toBe(screen.getByRole("option", { name: "Unreads" }));
Expand All @@ -119,21 +125,21 @@ describe("<RoomListPrimaryFilters /> stories", () => {
it("should restore original filter order when expanded", async () => {
const user = userEvent.setup();
render(<NarrowWithActiveWrappingFilter />);
mockUnreadWrapping();
await mockUnreadWrapping();

await user.click(screen.getByRole("button", { name: "Expand filter list" }));

const listbox = screen.getByRole("listbox", { name: "Room list filters" });
expect(listbox.children[0]).toBe(screen.getByRole("option", { name: "People" }));
});

it("should handle resize from non-wrapping to wrapping", () => {
it("should handle resize from non-wrapping to wrapping", async () => {
render(<NarrowContainer />);
mockFiltersNotWrapping();
await mockFiltersNotWrapping();

expect(screen.queryByRole("button", { name: "Expand filter list" })).toBeNull();

mockUnreadWrapping();
await mockUnreadWrapping();
expect(screen.getByRole("button", { name: "Expand filter list" })).toBeInTheDocument();
});
});
Expand Down
7 changes: 6 additions & 1 deletion packages/shared-components/src/room-list/RoomListPrimaryFilters/useCollapseFilters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,15 @@
};

hideFilters(ref.current);
const observer = new ResizeObserver((entries) => entries.forEach((entry) => hideFilters(entry.target)));
let rafId = 0;
const observer = new ResizeObserver((entries) => {
cancelAnimationFrame(rafId);
rafId = requestAnimationFrame(() => entries.forEach((entry) => hideFilters(entry.target)));

Check failure on line 65 in packages/shared-components/src/room-list/RoomListPrimaryFilters/useCollapseFilters.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this code to not nest functions more than 4 levels deep. 


See more on https://sonarcloud.io/project/issues?id=element-web&issues=AZzTxigFkZa_-9vKUSck&open=AZzTxigFkZa_-9vKUSck&pullRequest=32759
});

observer.observe(ref.current);
return () => {
cancelAnimationFrame(rafId);
observer.disconnect();
};
}, [isExpanded, wrappingClassName]);
Expand Down
2 changes: 1 addition & 1 deletion packages/shared-components/src/utils/i18n.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ interface ICounterpartTranslation {
}

async function getLanguage(langPath: string): Promise<ICounterpartTranslation> {
console.log("Loading language from", langPath);
if (process.env.NODE_ENV !== "test") console.log("Loading language from", langPath);

@t3chguy t3chguy Mar 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is not a great approach, the test should define the console logs it doesn't care about and ignore. In certain tests this being spammed may be a red flag one wouldn't want to hide and cannot mutate the environment variable without breaking other things

@florianduros florianduros Mar 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm a solution is to add a env var to disable log related to language in SC tests. But enabled the log in i18n tests of SC

@t3chguy t3chguy Mar 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree, if our types were correct env would be inaccessible due to it being part of the node types and SC isn't designed for node but for the web.

@florianduros florianduros Mar 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you suggest as solution?

@t3chguy t3chguy Mar 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I stated initially

IMO this is not a great approach, the test should define the console logs it doesn't care about and ignore.

https://github.com/search?q=repo%3Aelement-hq%2Felement-web%20filterConsole&type=code

@florianduros florianduros Mar 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep but this is spamming in all SC tests:

  • we add the filter in every testsuite except i18n -> kind of burden to do it
  • we put it globally in SC tests -> we lose this log in i18n

@t3chguy t3chguy Apr 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we put it globally in SC tests -> we lose this log in i18n

Which is what this PR does already given it uses NODE_ENV which is the same for all tests

const res = await fetch(langPath, { method: "GET" });

if (!res.ok) {
Expand Down
Loading