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.

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

Refactored src/components/ChangeLanguageDropdown/ChangeLanguageDropdown.test.tsx from Jest to Vitest #2588

Open
wants to merge 4 commits into
base: develop-postgres
Choose a base branch
from
Open

Refactored src/components/ChangeLanguageDropdown/ChangeLanguageDropdown.test.tsx from Jest to Vitest #2588

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d3243f6
Refactored src/components/ChangeLanguageDropdown/ChangeLanguageDropdo…
bitbard3 Dec 2, 2024
1b6d7a6
Add ts-doc comment
bitbard3 Dec 2, 2024
76c76b5
Merge branch 'develop-postgres' into rfac/ChangeLanguageDropdown-vitest
varshith257 Dec 8, 2024
9ad3b98
Merge branch 'develop-postgres' into rfac/ChangeLanguageDropdown-vitest
palisadoes Dec 24, 2024
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
26 changes: 16 additions & 10 deletions ...eDropdown/ChangeLanguageDropdown.test.tsx → ...eDropdown/ChangeLanguageDropdown.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
import React, { act } from 'react';
/**
* Unit tests for the ChangeLanguageDropdown component.
*
* This file contains tests for the ChangeLanguageDropdown component to ensure it behaves as expected
* under various scenarios.
*/
import React from 'react';
import { describe, test, expect } from 'vitest';
import { render } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { I18nextProvider } from 'react-i18next';
Expand All @@ -16,10 +23,8 @@ import useLocalStorage from 'utils/useLocalstorage';
const { setItem } = useLocalStorage();

async function wait(ms = 100): Promise<void> {
await act(() => {
return new Promise((resolve) => {
setTimeout(resolve, ms);
});
await new Promise((resolve) => {
setTimeout(resolve, ms);
});
}

Expand Down Expand Up @@ -51,6 +56,7 @@ const MOCKS = [
];

const link = new StaticMockLink(MOCKS, true);

describe('Testing Change Language Dropdown', () => {
test('Component Should be rendered properly', async () => {
const { getByTestId } = render(
Expand All @@ -71,7 +77,7 @@ describe('Testing Change Language Dropdown', () => {
getByTestId('language-dropdown-btn').className.includes('');
getByTestId('dropdown-btn-0').className.includes('');

userEvent.click(getByTestId('dropdown-btn-0'));
await userEvent.click(getByTestId('dropdown-btn-0'));
await wait();

languages.map((language) => {
Expand Down Expand Up @@ -136,23 +142,23 @@ describe('Testing Change Language Dropdown', () => {
</MockedProvider>,
);

userEvent.click(getByTestId('language-dropdown-btn'));
await userEvent.click(getByTestId('language-dropdown-btn'));
await wait();
const changeLanguageBtn = getByTestId(`change-language-btn-fr`);
await wait();
expect(changeLanguageBtn).toBeInTheDocument();
await wait();
userEvent.click(changeLanguageBtn);
await userEvent.click(changeLanguageBtn);
await wait();
expect(cookies.get('i18next')).toBe('fr');
await wait();
userEvent.click(getByTestId('language-dropdown-btn'));
await userEvent.click(getByTestId('language-dropdown-btn'));
await wait();
Comment on lines +145 to +155
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize test structure by reducing unnecessary waits

The current implementation has multiple consecutive wait() calls that could make the tests flaky and slower than necessary. Consider consolidating the waits and using more reliable ways to wait for state changes.

-    await userEvent.click(getByTestId('language-dropdown-btn'));
-    await wait();
-    const changeLanguageBtn = getByTestId(`change-language-btn-fr`);
-    await wait();
-    expect(changeLanguageBtn).toBeInTheDocument();
-    await wait();
-    await userEvent.click(changeLanguageBtn);
-    await wait();
-    expect(cookies.get('i18next')).toBe('fr');
-    await wait();
+    await userEvent.click(getByTestId('language-dropdown-btn'));
+    const changeLanguageBtn = getByTestId(`change-language-btn-fr`);
+    expect(changeLanguageBtn).toBeInTheDocument();
+    await userEvent.click(changeLanguageBtn);
+    // Wait for cookie to be set
+    await wait(200);
+    expect(cookies.get('i18next')).toBe('fr');
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await userEvent.click(getByTestId('language-dropdown-btn'));
await wait();
const changeLanguageBtn = getByTestId(`change-language-btn-fr`);
await wait();
expect(changeLanguageBtn).toBeInTheDocument();
await wait();
userEvent.click(changeLanguageBtn);
await userEvent.click(changeLanguageBtn);
await wait();
expect(cookies.get('i18next')).toBe('fr');
await wait();
userEvent.click(getByTestId('language-dropdown-btn'));
await userEvent.click(getByTestId('language-dropdown-btn'));
await userEvent.click(getByTestId('language-dropdown-btn'));
const changeLanguageBtn = getByTestId(`change-language-btn-fr`);
expect(changeLanguageBtn).toBeInTheDocument();
await userEvent.click(changeLanguageBtn);
// Wait for cookie to be set
await wait(200);
expect(cookies.get('i18next')).toBe('fr');

Copy link
Author

Choose a reason for hiding this comment

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

Not relevant to this issue, This PR only refactors the test case to vitest

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood @bitbard3, thank you for the clarification. I'll keep that in mind for future reviews.

✏️ Learnings added 
Learnt from: bitbard3
PR: PalisadoesFoundation/talawa-admin#2588
File: src/components/ChangeLanguageDropdown/ChangeLanguageDropdown.spec.tsx:145-155
Timestamp: 2024-12-02T04:20:11.616Z
Learning: In PRs focused solely on refactoring test cases from Jest to Vitest, avoid suggesting optimizations or changes outside the migration scope.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Member

Choose a reason for hiding this comment

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

@bitbard3 Multiple wait calls make redundancy. Please apply the coderabbitai changes

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

const changeLanguageBtnHi = getByTestId(`change-language-btn-hi`);
await wait();
expect(changeLanguageBtnHi).toBeInTheDocument();
await wait();
userEvent.click(changeLanguageBtnHi);
await userEvent.click(changeLanguageBtnHi);
await wait();
});
Comment on lines +155 to 162
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add assertions for error handling

The test case for the Hindi language selection triggers an error (as per the mock), but doesn't verify how the error is handled. Consider adding assertions to validate the error state.

     await userEvent.click(getByTestId('language-dropdown-btn'));
-    await wait();
     const changeLanguageBtnHi = getByTestId(`change-language-btn-hi`);
-    await wait();
     expect(changeLanguageBtnHi).toBeInTheDocument();
-    await wait();
     await userEvent.click(changeLanguageBtnHi);
-    await wait();
+    await wait(200);
+    // Add assertions for error handling
+    expect(cookies.get('i18next')).not.toBe('hi');
+    // Verify error message is displayed (add appropriate test ID)
+    expect(getByTestId('error-message')).toBeInTheDocument();

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Author

Choose a reason for hiding this comment

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

Hello reviewer this changes are outside the scope of this issue, Do you still want me to add these changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bitbard3, understood. Since these changes are outside the scope of this issue, it's acceptable to leave them as is. Would you like me to open a new GitHub issue to track adding these error handling assertions in the future?

🧠 Learnings used 
Learnt from: bitbard3
PR: PalisadoesFoundation/talawa-admin#2588
File: src/components/ChangeLanguageDropdown/ChangeLanguageDropdown.spec.tsx:145-155
Timestamp: 2024-12-02T04:20:11.745Z
Learning: In PRs focused solely on refactoring test cases from Jest to Vitest, avoid suggesting optimizations or changes outside the migration scope.

Copy link
Author

Choose a reason for hiding this comment

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

Error handling has not been implemented in the component, so the test case does not cover error scenarios

Copy link
Contributor

Choose a reason for hiding this comment

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

@bitbard3, understood. Thank you for the clarification. Since error handling is not implemented in the component, it's acceptable that the test case does not cover error scenarios.

✏️ Learnings added 
Learnt from: bitbard3
PR: PalisadoesFoundation/talawa-admin#2588
File: src/components/ChangeLanguageDropdown/ChangeLanguageDropdown.spec.tsx:155-162
Timestamp: 2024-12-03T05:52:37.748Z
Learning: In the `ChangeLanguageDropdown` component (`src/components/ChangeLanguageDropdown/ChangeLanguageDropdown.tsx`), error handling has not been implemented. Therefore, test cases in `src/components/ChangeLanguageDropdown/ChangeLanguageDropdown.spec.tsx` do not cover error scenarios related to error handling.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: bitbard3
PR: PalisadoesFoundation/talawa-admin#2588
File: src/components/ChangeLanguageDropdown/ChangeLanguageDropdown.spec.tsx:145-155
Timestamp: 2024-12-02T04:20:11.745Z
Learning: In PRs focused solely on refactoring test cases from Jest to Vitest, avoid suggesting optimizations or changes outside the migration scope.

Copy link
Member

Choose a reason for hiding this comment

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

Here too....

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

});
Loading