Add AdvertisementEntry.spec.tsx for Vitest migration #2680
Conversation
WalkthroughThis pull request introduces a new test specification file Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx (6)
29-34: Handle invalid token in authorization header.
If getItem('token') returns null or undefined, it will produce "Bearer null" or "Bearer undefined". Consider gracefully handling missing tokens.-headers: { - authorization: 'Bearer ' + getItem('token') || '', -}, +headers: { + authorization: getItem('token') + ? `Bearer ${getItem('token')}` + : '', +},
69-142: Enhance negative scenario validation.
The tests properly verify that “Advertisement Deleted” does not appear in unsuccessful deletion. However, to improve clarity, consider explicitly checking for error UI feedback or mocked error logging so that you confirm correct handling of the error state—rather than verifying absence of a success message alone.
203-248: Extract repeated dropdown interactions into a helper function.
Multiple tests open and close the dropdown similarly. Moving repeated interaction code to a helper or utility function can reduce duplication, improve readability, and ease future maintenance.
327-385: Confirm error-handling coverage.
This test covers the scenario where the mutation’s “data” field is missing or unexpected. Consider adding an explicit assertion for error states or logs to confirm correct handling.
387-461: Improve feedback for missing data on registration.
This test ensures the code handles missing “data” from the createAdvertisement mutation. However, consider verifying that the user receives a suitable error message or fallback UI if that situation arises in real usage.
464-647: Reduce duplication between the rendering & deletion tests.
Lines 464-647 closely mirror the earlier “rendering and deleting advertisement” test with the addition of MockedProvider. Extract the common setup into a shared utility or helper function for more maintainable tests.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx(1 hunks)
🔇 Additional comments (1)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx (1)
170-170: Confirm “Ends on” string usage.
Line 170 has a “fix text 'Ends on'?” comment. Ensure consistency across code and tests. For instance, if the UI should display “Starts on”, then consider adjusting the text.
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx (3)
29-34: Consider enhancing token security in testsThe current implementation directly accesses localStorage through
getItem. Consider mocking localStorage or using a test-specific token to avoid potential security issues in tests.- headers: { - authorization: 'Bearer ' + getItem('token') || '', - }, + headers: { + authorization: 'Bearer test-token', + },
42-45: Consider moving Apollo Client setup to a test utilityThe Apollo Client configuration could be moved to a shared test utility file to promote reusability across test files.
248-248: Improve test descriptionsThe test descriptions could be more specific about what they're testing:
- "Updates the advertisement and shows success toast on successful update" -> "should successfully update advertisement with valid data and display success message"
- "Simulating if the mutation doesnt have data variable while updating" -> "should handle missing data in update mutation response"
- "Simulating if the mutation does not have data variable while registering" -> "should handle missing data in registration mutation response"
Also applies to: 325-325, 385-385
🧰 Tools
🪛 Biome (1.9.4)
[error] 248-248: Unexpected token. Did you mean
{'>'}or>?(parse)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx
[error] 148-148: Expected a function body but instead found '_value'.
Expected a function body here.
(parse)
[error] 148-148: Expected a JSX attribute but instead found '.'.
Expected a JSX attribute here.
(parse)
[error] 148-148: expected > but instead found <
Remove <
(parse)
[error] 148-148: Expected a JSX attribute but instead found '| null | undefined'.
Expected a JSX attribute here.
(parse)
[error] 150-150: expected } but instead found throw
Remove throw
(parse)
[error] 151-151: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 151-151: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 152-152: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 157-157: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 170-170: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 172-172: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 173-173: expected } but instead found const
Remove const
(parse)
[error] 189-189: Expected a function body but instead found '_value'.
Expected a function body here.
(parse)
[error] 189-189: Expected a JSX attribute but instead found '.'.
Expected a JSX attribute here.
(parse)
[error] 189-189: expected > but instead found <
Remove <
(parse)
[error] 189-189: Expected a JSX attribute but instead found '| null | undefined'.
Expected a JSX attribute here.
(parse)
[error] 191-191: expected } but instead found throw
Remove throw
(parse)
[error] 192-192: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 192-192: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 193-193: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 199-199: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 201-201: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 202-202: expected } but instead found const
Remove const
(parse)
[error] 246-246: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 248-248: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 249-249: expected } but instead found const
Remove const
(parse)
[error] 250-250: expected } but instead found :
Remove :
(parse)
[error] 251-251: expected } but instead found :
Remove :
(parse)
[error] 252-252: expected } but instead found :
Remove :
(parse)
[error] 253-253: expected } but instead found :
Remove :
(parse)
[error] 259-259: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 260-260: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 261-261: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 262-262: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 292-292: expected } but instead found :
Remove :
(parse)
[error] 292-292: expected } but instead found :
Remove :
(parse)
[error] 292-292: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 293-293: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 300-300: expected } but instead found :
Remove :
(parse)
[error] 300-300: expected } but instead found :
Remove :
(parse)
[error] 300-300: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 301-301: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 305-305: expected } but instead found :
Remove :
(parse)
[error] 305-305: expected } but instead found :
Remove :
(parse)
[error] 305-305: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 306-306: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 309-309: expected } but instead found :
Remove :
(parse)
[error] 309-309: expected } but instead found :
Remove :
(parse)
[error] 309-309: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 310-310: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 315-315: expected } but instead found :
Remove :
(parse)
[error] 316-316: expected } but instead found :
Remove :
(parse)
[error] 321-321: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 322-322: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 323-323: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 325-325: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 326-326: expected } but instead found const
Remove const
(parse)
[error] 327-327: expected } but instead found :
Remove :
(parse)
[error] 328-328: expected } but instead found :
Remove :
(parse)
[error] 331-331: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 332-332: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 362-362: expected } but instead found :
Remove :
(parse)
[error] 362-362: expected } but instead found :
Remove :
(parse)
[error] 362-362: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 363-363: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 370-370: expected } but instead found :
Remove :
(parse)
[error] 370-370: expected } but instead found :
Remove :
(parse)
[error] 370-370: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 371-371: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 377-377: expected } but instead found :
Remove :
(parse)
[error] 378-378: expected } but instead found :
Remove :
(parse)
[error] 381-381: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 382-382: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 383-383: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 385-385: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 389-389: expected } but instead found ;
Remove ;
(parse)
[error] 392-392: expected } but instead found :
Remove :
(parse)
[error] 393-393: expected } but instead found :
Remove :
(parse)
[error] 394-394: expected } but instead found :
Remove :
(parse)
[error] 395-395: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 396-396: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 397-397: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 421-421: expected } but instead found :
Remove :
(parse)
[error] 421-421: expected } but instead found :
Remove :
(parse)
[error] 421-421: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 422-422: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 429-429: expected } but instead found :
Remove :
(parse)
[error] 429-429: expected } but instead found :
Remove :
(parse)
[error] 429-429: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 430-430: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 434-434: expected } but instead found :
Remove :
(parse)
[error] 434-434: expected } but instead found :
Remove :
(parse)
[error] 434-434: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 435-435: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 441-441: expected } but instead found :
Remove :
(parse)
[error] 441-441: expected } but instead found :
Remove :
(parse)
[error] 441-441: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 442-442: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 450-450: expected } but instead found :
Remove :
(parse)
[error] 451-451: expected } but instead found :
Remove :
(parse)
[error] 457-457: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 458-458: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 459-459: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 461-461: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 462-462: expected } but instead found const
Remove const
(parse)
[error] 465-465: expected } but instead found :
Remove :
(parse)
[error] 466-466: expected } but instead found :
Remove :
(parse)
[error] 468-468: expected } but instead found :
Remove :
(parse)
[error] 473-473: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 474-474: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 476-476: expected } but instead found :
Remove :
(parse)
[error] 477-477: expected } but instead found :
Remove :
(parse)
[error] 479-479: expected } but instead found :
Remove :
(parse)
[error] 481-481: expected } but instead found :
Remove :
(parse)
[error] 483-483: expected } but instead found :
Remove :
(parse)
[error] 484-484: expected } but instead found :
Remove :
(parse)
[error] 489-489: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 491-491: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 493-493: expected } but instead found :
Remove :
(parse)
[error] 494-494: expected } but instead found :
Remove :
(parse)
[error] 499-499: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 501-501: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 503-503: expected } but instead found :
Remove :
(parse)
[error] 504-504: expected } but instead found :
Remove :
(parse)
[error] 509-509: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 511-511: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 513-513: expected } but instead found :
Remove :
(parse)
[error] 514-514: expected } but instead found :
Remove :
(parse)
[error] 519-519: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 521-521: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 523-523: expected } but instead found :
Remove :
(parse)
[error] 524-524: expected } but instead found :
Remove :
(parse)
[error] 529-529: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 531-531: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 533-533: expected } but instead found :
Remove :
(parse)
[error] 534-534: expected } but instead found :
Remove :
(parse)
[error] 539-539: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 541-541: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 544-544: expected } but instead found :
Remove :
(parse)
[error] 548-548: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 550-550: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 551-551: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 553-553: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 554-554: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 555-555: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 557-557: expected } but instead found :
Remove :
(parse)
[error] 558-558: expected } but instead found :
Remove :
(parse)
[error] 560-560: expected } but instead found :
Remove :
(parse)
[error] 561-561: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 562-562: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 564-564: expected } but instead found :
Remove :
(parse)
[error] 565-565: expected } but instead found :
Remove :
(parse)
[error] 566-566: expected } but instead found :
Remove :
(parse)
[error] 567-567: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 568-568: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 569-569: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 570-570: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 607-607: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 608-608: expected } but instead found ;
Remove ;
(parse)
[error] 610-610: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 614-614: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 619-619: expected } but instead found ;
Remove ;
(parse)
[error] 622-622: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 631-631: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 636-636: expected } but instead found ;
Remove ;
(parse)
[error] 637-637: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 638-638: expected } but instead found return
Remove return
(parse)
[error] 642-642: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 644-644: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 645-645: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 645-646: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 646-646: expected < but instead the file ends
the file ends here
(parse)
[error] 155-156: Wrap comments inside children within braces.
Unsafe fix: Wrap the comments with braces
(lint/suspicious/noCommentText)
[error] 167-167: Wrap comments inside children within braces.
Unsafe fix: Wrap the comments with braces
(lint/suspicious/noCommentText)
[error] 169-169: Wrap comments inside children within braces.
Unsafe fix: Wrap the comments with braces
(lint/suspicious/noCommentText)
[error] 196-197: Wrap comments inside children within braces.
Unsafe fix: Wrap the comments with braces
(lint/suspicious/noCommentText)
[error] 224-224: Wrap comments inside children within braces.
Unsafe fix: Wrap the comments with braces
(lint/suspicious/noCommentText)
[error] 597-597: Wrap comments inside children within braces.
Unsafe fix: Wrap the comments with braces
(lint/suspicious/noCommentText)
[error] 624-624: Wrap comments inside children within braces.
Unsafe fix: Wrap the comments with braces
(lint/suspicious/noCommentText)
🔇 Additional comments (2)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx (2)
24-24: Successfully migrated to Vitest
The migration from Jest to Vitest has been implemented correctly, maintaining proper test coverage and correctly utilizing Vitest's API.
156-157:
Improve empty string assertions
Testing for empty strings using getAllByText('') could lead to false positives by matching unrelated empty text nodes. Consider testing specific elements or placeholders.
- const elements = screen.getAllByText(''); // This will return an array of matching elements
- elements.forEach((element) => expect(element).toBeInTheDocument());
+ expect(screen.getByTestId('name')).toHaveValue('');
+ expect(screen.getByTestId('type')).toHaveValue('');Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 157-157: Unexpected token. Did you mean {'>'} or >?
(parse)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx
Show resolved
Hide resolved
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx (3)
170-170:⚠️ Potential issueFix incorrect assertion text for default start date
The assertion text uses "Ends on" for the start date, which is incorrect.
-expect(screen.getByText(`Ends on ${defaultStartDate}`)).toBeInTheDocument(); +expect(screen.getByText(`Starts on ${defaultStartDate}`)).toBeInTheDocument();
157-158:⚠️ Potential issueAvoid checking multiple empty text matches at once
Using
getAllByText('')may incorrectly match multiple empty strings that are not relevant to the test.-const elements = screen.getAllByText(''); -elements.forEach((element) => expect(element).toBeInTheDocument()); +expect(screen.getByTestId('name')).toHaveTextContent(''); +expect(screen.getByTestId('type')).toHaveTextContent('');
463-647:⚠️ Potential issueRemove duplicate deletion test case
This test case appears to be a duplicate of the deletion test at lines 69-142. The functionality is already covered in the first test case.
Consider removing this duplicate test case or consolidating unique scenarios into the original test.
🧹 Nitpick comments (2)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx (2)
47-62: Consider moving mock setup to a separate test utils fileThe mock implementations for Apollo Client and React Router could be reused across different test files.
Create a new file
__tests__/utils/mocks.ts:export const mockApolloClient = (mockUseMutation: jest.Mock) => { return { useMutation: () => mockUseMutation(), }; }; export const mockReactRouter = () => { return { useParams: () => ({ orgId: '1' }), }; };
149-149: Remove unused parameter warningsThe
_valueparameter in thesetAfterfunction is defined but never used.-_value: React.SetStateAction<string | null | undefined> +value: React.SetStateAction<string | null | undefined>Also applies to: 191-191
🧰 Tools
🪛 eslint
[error] 149-149: '_value' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 149-149: Delete
·(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx
[error] 149-149: '_value' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 149-149: Delete ·
(prettier/prettier)
[error] 191-191: '_value' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 191-191: Delete ·
(prettier/prettier)
[error] 192-192: Replace ··)· with )
(prettier/prettier)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx
Show resolved
Hide resolved
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx
Show resolved
Hide resolved
|
Please fix:
Make sure CodeRabbit.ai approves your changes |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx
[error] 148-149: Replace ⏎········· with )
(prettier/prettier)
[error] 150-150: Delete )
(prettier/prettier)
[error] 190-191: Replace ⏎········· with )
(prettier/prettier)
[error] 192-192: Delete )
(prettier/prettier)
🔇 Additional comments (3)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx (3)
170-170:
Fix incorrect assertion text
The test is using "Ends on" for both start and end dates, which appears to be incorrect.
- expect(screen.getByText(`Ends on ${defaultStartDate}`)).toBeInTheDocument();
+ expect(screen.getByText(`Starts on ${defaultStartDate}`)).toBeInTheDocument();Likely invalid or redundant comment.
29-34:
Security: Avoid exposing authentication token in HTTP headers
The current implementation directly accesses the token from localStorage in the HTTP link configuration, which could expose sensitive information if the Apollo Client configuration is logged or serialized.
Consider using an auth link interceptor:
-const httpLink = new HttpLink({
- uri: BACKEND_URL,
- headers: {
- authorization: 'Bearer ' + getItem('token') || '',
- },
-});
+const authLink = new ApolloLink((operation, forward) => {
+ const token = getItem('token');
+ operation.setContext({
+ headers: {
+ authorization: token ? `Bearer ${token}` : '',
+ },
+ });
+ return forward(operation);
+});
+
+const httpLink = new HttpLink({ uri: BACKEND_URL });Likely invalid or redundant comment.
198-201: 🛠️ Refactor suggestion
Add missing assertions in override props test
The test case for overriding default props is incomplete. Consider adding assertions for all provided props.
expect(getByText(mockName)).toBeInTheDocument();
+ expect(getByText(mockType)).toBeInTheDocument();
+ expect(screen.getByTestId('media')).toHaveAttribute('src', mockMediaUrl);
+ expect(getByText(`Ends on ${mockEndDate.toDateString()}`)).toBeInTheDocument();
+ expect(getByText(`Starts on ${mockStartDate.toDateString()}`)).toBeInTheDocument();Likely invalid or redundant comment.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2680 +/- ##
====================================================
+ Coverage 86.10% 86.36% +0.26%
====================================================
Files 295 312 +17
Lines 7259 8134 +875
Branches 1591 1785 +194
====================================================
+ Hits 6250 7025 +775
- Misses 781 932 +151
+ Partials 228 177 -51 ☔ View full report in Codecov by Sentry. |
|
Please fix:
Make sure CodeRabbit.ai approves your changes |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx (1)
156-157: Avoid checking multiple empty text matches at onceUsing
getAllByText('')may incorrectly match multiple empty strings that are not relevant.Instead, verify specific elements or placeholders that confirm correct rendering of defaults:
- const elements = screen.getAllByText(''); // This will return an array of matching elements - elements.forEach((element) => expect(element).toBeInTheDocument()); + expect(screen.getByTestId('name-field')).toHaveValue(''); + expect(screen.getByTestId('type-field')).toHaveValue('');
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx
[failure] 12-12:
Cannot find module './core/AdvertisementEntry/AdvertisementEntry' or its corresponding type declarations.
[failure] 13-13:
Cannot find module './core/AdvertisementRegister/AdvertisementRegister' or its corresponding type declarations.
🔇 Additional comments (3)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx (3)
169-169:
Fix incorrect assertion text for start date
The test is using "Ends on" for both start and end dates, which is incorrect.
- expect(screen.getByText(`Ends on ${defaultStartDate}`)).toBeInTheDocument();
+ expect(screen.getByText(`Starts on ${defaultStartDate}`)).toBeInTheDocument();Likely invalid or redundant comment.
29-34:
Security: Avoid exposing authentication token in HTTP headers
The current implementation directly accesses the token from localStorage in the HTTP link configuration. This could expose sensitive information if the Apollo Client configuration is logged or serialized.
Consider using an auth link interceptor:
-const httpLink = new HttpLink({
- uri: BACKEND_URL,
- headers: {
- authorization: 'Bearer ' + getItem('token') || '',
- },
-});
+const authLink = new ApolloLink((operation, forward) => {
+ const token = getItem('token');
+ operation.setContext({
+ headers: {
+ authorization: token ? `Bearer ${token}` : '',
+ },
+ });
+ return forward(operation);
+});
+
+const httpLink = new HttpLink({ uri: BACKEND_URL });Likely invalid or redundant comment.
196-199: 🛠️ Refactor suggestion
Add missing assertions in override props test
The test case for overriding default props is incomplete. It should verify all provided props.
Add assertions for all provided props:
expect(getByText(mockName)).toBeInTheDocument();
+ expect(getByText(mockType)).toBeInTheDocument();
+ expect(screen.getByTestId('media')).toHaveAttribute('src', mockMediaUrl);
+ expect(getByText(`Ends on ${mockEndDate.toDateString()}`)).toBeInTheDocument();
+ expect(getByText(`Starts on ${mockStartDate.toDateString()}`)).toBeInTheDocument();Likely invalid or redundant comment.
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx
[failure] 12-12:
Cannot find module '/AdvertisementEntry' or its corresponding type declarations.
[failure] 13-13:
Cannot find module './core/AdvertisementRegister/AdvertisementRegister' or its corresponding type declarations.
🔇 Additional comments (4)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx (4)
196-199: 🛠️ Refactor suggestion
Add missing assertions in override props test
The test case for overriding default props is incomplete. Consider adding assertions for all provided props.
expect(getByText(mockName)).toBeInTheDocument();
+ expect(getByText(mockType)).toBeInTheDocument();
+ expect(screen.getByTestId('media')).toHaveAttribute('src', mockMediaUrl);
+ expect(getByText(`Ends on ${mockEndDate.toDateString()}`)).toBeInTheDocument();
+ expect(getByText(`Starts on ${mockStartDate.toDateString()}`)).toBeInTheDocument();Likely invalid or redundant comment.
169-169:
Fix incorrect assertion text
The test is using "Ends on" for both start and end dates, which appears to be incorrect.
- expect(screen.getByText(`Ends on ${defaultStartDate}`)).toBeInTheDocument();
+ expect(screen.getByText(`Starts on ${defaultStartDate}`)).toBeInTheDocument();Likely invalid or redundant comment.
29-34:
Security: Avoid exposing authentication token in HTTP headers
The current implementation directly exposes the token in the Apollo Client configuration. This could lead to token exposure if the configuration is logged or serialized.
Use an auth link interceptor instead:
-const httpLink = new HttpLink({
- uri: BACKEND_URL,
- headers: {
- authorization: 'Bearer ' + getItem('token') || '',
- },
-});
+const authLink = new ApolloLink((operation, forward) => {
+ const token = getItem('token');
+ operation.setContext({
+ headers: {
+ authorization: token ? `Bearer ${token}` : '',
+ },
+ });
+ return forward(operation);
+});
+
+const httpLink = new HttpLink({ uri: BACKEND_URL });Likely invalid or redundant comment.
12-13:
Fix incorrect import paths
The import paths for AdvertisementEntry and AdvertisementRegister components are incorrect and will cause compilation errors.
Apply this diff to fix the import paths:
-import AdvertisementEntry from '/AdvertisementEntry';
-import AdvertisementRegister from './core/AdvertisementRegister/AdvertisementRegister';
+import AdvertisementEntry from '../AdvertisementEntry';
+import AdvertisementRegister from '../AdvertisementRegister';Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[failure] 12-12:
Cannot find module '/AdvertisementEntry' or its corresponding type declarations.
[failure] 13-13:
Cannot find module './core/AdvertisementRegister/AdvertisementRegister' or its corresponding type declarations.
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx
Show resolved
Hide resolved
IITI-tushar
force-pushed
the
AdvertisementEntrySpec-jest-to-vitest
branch
from
e6175be to
c239dc0
Compare
|
@palisadoes I have made changes according to AdvertisementEntry.test.tsx , and coderabbitbot is asking me to make changes which conflict the AdvertisementEntry.test.tsx , So should I go with the changes coderabbit is suggesting , or just resolve the conversation? |
|
@IITI-tushar Please address the coderabbit comments and fix the failed tests. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx (1)
154-156: Improve empty text matching assertionUsing
getAllByText('')may incorrectly match multiple empty strings that are not relevant to the test.- const elements = screen.getAllByText(''); // This will return an array of matching elements - elements.forEach((element) => expect(element).toBeInTheDocument()); + // Instead, verify specific elements or placeholders that confirm correct rendering of defaults + expect(screen.getByTestId('name-field')).toHaveValue(''); + expect(screen.getByTestId('type-field')).toHaveValue('');
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx
[error] 30-30: Delete ··
(prettier/prettier)
[error] 31-31: Delete ··
(prettier/prettier)
[error] 32-32: Delete ··
(prettier/prettier)
[error] 33-33: Delete ··
(prettier/prettier)
[error] 34-34: Delete ··
(prettier/prettier)
[error] 168-170: Replace ⏎······screen.getByText(Ends·on·${defaultStartDate}),⏎···· with screen.getByText(Ends·on·${defaultStartDate})
(prettier/prettier)
[error] 375-375: Delete ·
(prettier/prettier)
🔇 Additional comments (2)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx (2)
169-170:
Fix incorrect assertion text for start date
The test is using "Ends on" for both start and end dates, which is incorrect.
- expect(
- screen.getByText(`Ends on ${defaultStartDate}`),
- ).toBeInTheDocument();
+ expect(screen.getByText(`Starts on ${defaultStartDate}`)).toBeInTheDocument();Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 168-170: Replace ⏎······screen.getByText(Ends·on·${defaultStartDate}),⏎···· with screen.getByText(Ends·on·${defaultStartDate})
(prettier/prettier)
29-34: 🛠️ Refactor suggestion
Enhance security of token handling in Apollo Client setup
The current implementation directly exposes the authentication token in the HTTP link configuration. Consider using an auth link interceptor for more secure token handling.
-const httpLink = new HttpLink({
- uri: BACKEND_URL,
- headers: {
- authorization: 'Bearer ' + getItem('token') || '',
- },
- });
+const authLink = new ApolloLink((operation, forward) => {
+ const token = getItem('token');
+ operation.setContext({
+ headers: {
+ authorization: token ? `Bearer ${token}` : '',
+ },
+ });
+ return forward(operation);
+});
+
+const httpLink = new HttpLink({ uri: BACKEND_URL });Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 30-30: Delete ··
(prettier/prettier)
[error] 31-31: Delete ··
(prettier/prettier)
[error] 32-32: Delete ··
(prettier/prettier)
[error] 33-33: Delete ··
(prettier/prettier)
[error] 34-34: Delete ··
(prettier/prettier)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx (1)
251-326: Add assertions for success notificationsThe update test should verify that a success toast/notification is shown to the user after a successful update.
fireEvent.click(screen.getByTestId('addonupdate')); expect(updateAdByIdMock).toHaveBeenCalledWith({ variables: { id: '1', name: 'Updated Advertisement', type: 'BANNER', startDate: dayjs().add(1, 'day').format('YYYY-MM-DD'), endDate: dayjs().add(2, 'days').format('YYYY-MM-DD'), }, }); + await waitFor(() => { + expect(screen.getByText('Advertisement Updated Successfully')).toBeInTheDocument(); + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx
[warning] 328-328:
Some tests seem to be commented
🔇 Additional comments (2)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx (2)
29-34: Avoid exposing authentication token in HTTP headers
The current implementation directly accesses the token from localStorage in the HTTP link configuration, which could expose sensitive information.
168-168:
Fix incorrect assertion text
The test is using "Ends on" for the start date, which is incorrect.
- expect(screen.getByText(`Ends on ${defaultStartDate}`)).toBeInTheDocument();
+ expect(screen.getByText(`Starts on ${defaultStartDate}`)).toBeInTheDocument();Likely invalid or redundant comment.
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx
Outdated
Show resolved
Hide resolved
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx
Show resolved
Hide resolved
|
@Cioppolo14 @palisadoes I addressed code rabbit comments. and all tests are passed |
@palisadoes |
palisadoes
merged commit 3c6f6da
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
refactoring
Issue Number: #2484
Fixes ##2484
Did you add tests for your changes?
Vitest tes passed :
coverage for AdvertisementEntry.spec.tsx file
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
Replace Jest-specific functions and mocks with Vitest equivalents and rename from test.tsx to spec.tsx
Does this PR introduce a breaking change?
Other information
Have you read the contributing guide?
yes
Summary by CodeRabbit