PLT-9674: Added Landing page unit and integration tests #73
Conversation
Coverage ReportSummary
JUnit
Coverage (10%)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (10)
- README.md (1 hunks)
- src/pages/landing/components/ConnectSection.test.tsx (2 hunks)
- src/pages/landing/components/RegisterModal.test.tsx (1 hunks)
- src/pages/landing/components/RegisterModal.tsx (1 hunks)
- src/pages/landing/components/RegisterSection.test.tsx (1 hunks)
- src/pages/landing/components/RegisterSection.tsx (1 hunks)
- src/pages/landing/components/SubscriptionSection.test.tsx (1 hunks)
- src/pages/landing/index.test.tsx (1 hunks)
- src/pages/landing/index.tsx (2 hunks)
- src/store/slices/walletConnection.slice.ts (4 hunks)
Additional comments: 11
src/pages/landing/components/RegisterModal.tsx (1)
- 16-16: Adding
role="dialog"enhances accessibility by informing screen readers that the component is a dialog. This is a good practice for web accessibility.src/pages/landing/components/ConnectSection.test.tsx (2)
- 10-10: Changing the description of the test suite to 'Landing page > Connection section' improves clarity and organization, making it easier to understand the scope of the tests.
- 11-19: Adding a test to check for the visibility of the wallet connect button is a good practice. It ensures that critical UI elements are rendered as expected.
README.md (1)
- 5-5: Updating the code coverage metrics in the README.md reflects the improvements made by the new tests. This is important for maintaining accurate documentation of the project's test coverage.
src/pages/landing/components/SubscriptionSection.test.tsx (1)
- 42-66: Reorganizing tests using
describeblocks and refining test descriptions enhances the readability and maintainability of the test suite. This structured approach makes it easier to navigate and understand the tests.src/pages/landing/index.tsx (1)
- 9-9: Adding the import statement for
PaymentDetailsVerificationfrom a more appropriate location improves code organization and avoids duplication. Ensure that the duplicate import statement has been removed as intended.src/pages/landing/components/RegisterModal.test.tsx (1)
- 10-120: The tests for
RegisterModalcover various scenarios, including rendering based on props and user interactions. These tests ensure the component behaves as expected under different conditions.src/pages/landing/components/RegisterSection.test.tsx (1)
- 33-105: The tests for
RegisterSectioneffectively cover rendering the section, displaying subscription prices, handling disabled forms, and form submission. These tests ensure the component functions correctly across various scenarios.src/pages/landing/index.test.tsx (1)
- 113-155: The integration tests for the landing page cover critical user flows, including wallet connection, subscription selection, and payment verification. These tests are crucial for ensuring the landing page functions as intended for end-users.
src/pages/landing/components/RegisterSection.tsx (1)
- 41-46: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The removal of the
eslint-disable-next-line react-hooks/exhaustive-depscomment suggests an improvement in adhering to best practices for dependency arrays inuseEffecthooks. Ensure that the component's behavior remains consistent and that there are no unintended side effects due to this change. Thorough testing is recommended to catch any subtle issues.src/store/slices/walletConnection.slice.ts (1)
- 66-66: Moving the
CardanoNSdeclaration to a local scope within theconnectWalletandstartListenWalletChangesfunctions is a good practice for improving code readability and maintainability. Ensure that the functionality of these functions remains unchanged and that there are no regressions due to this modification. Testing these functions thoroughly is recommended.
|
I cleaned up the PR to have it consistent with the PR template, filled the checklit and changed the commit message to be compatible with Conventional Commit (this is why I had to force push, I changed nothing else) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yaml
Files selected for processing (10)
- README.md (1 hunks)
- src/pages/landing/components/ConnectSection.test.tsx (2 hunks)
- src/pages/landing/components/RegisterModal.test.tsx (1 hunks)
- src/pages/landing/components/RegisterModal.tsx (1 hunks)
- src/pages/landing/components/RegisterSection.test.tsx (1 hunks)
- src/pages/landing/components/RegisterSection.tsx (1 hunks)
- src/pages/landing/components/SubscriptionSection.test.tsx (1 hunks)
- src/pages/landing/index.test.tsx (1 hunks)
- src/pages/landing/index.tsx (2 hunks)
- src/store/slices/walletConnection.slice.ts (4 hunks)
Files skipped from review as they are similar to previous changes (8)
- src/pages/landing/components/ConnectSection.test.tsx
- src/pages/landing/components/RegisterModal.test.tsx
- src/pages/landing/components/RegisterSection.test.tsx
- src/pages/landing/components/RegisterSection.tsx
- src/pages/landing/components/SubscriptionSection.test.tsx
- src/pages/landing/index.test.tsx
- src/pages/landing/index.tsx
- src/store/slices/walletConnection.slice.ts
Additional comments: 5
src/pages/landing/components/RegisterModal.tsx (1)
- 16-16: Adding
role="dialog"to theDialogcomponent is a good practice for accessibility, making it easier for screen reader users to understand the UI structure. Well done on this enhancement.README.md (4)
- 5-5: The update to the code coverage metrics in the README.md reflects significant improvements in testing coverage. This is a positive step towards ensuring higher code quality and reliability.
- 2-8: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [20-20]
Consider adding a comma for clarity: "To override the API endpoint, you have two options:"
- 2-8: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [22-22]
The term "env var" could be spelled out as "environment variable" for clarity, especially for readers who might not be familiar with the abbreviation.
- 2-8: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [60-60]
A comma seems to be missing after "docker". Consider revising to: "Production docker, NOTE: from the root of the project, run all the docker-files scripts."
| await delay(150); | ||
| return HttpResponse.json(-1); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Can we separate integration tests by some means, either into a different file with a different naming convention. As I think, we don't want to calculate the code coverage including them.
There was a problem hiding this comment.
I moved the integration test to another folder, but I don't know if we should exclude them from the code coverage calculation. Mostly because the integration tests are the ones that tests the reducers
| jest.mock('store/slices/walletTransaction.slice', () => ({ | ||
| payFromWallet: () => (() => ({ payload: 'transaction-id' })) | ||
| })); | ||
|
|
There was a problem hiding this comment.
Also keep mocks into a separate file , for easiness of maintainability
There was a problem hiding this comment.
I agree, but I'm struggling to make it work. Can we separate it in another Jira task?
amnambiar
left a comment
amnambiar
left a comment
There was a problem hiding this comment.
Suggestions added above
WalkthroughThe recent changes focus on enhancing the testing infrastructure and codebase organization within the landing page components of a web application. These improvements include increased code coverage metrics displayed in the README, refined test suites with precise user interaction testing, better encapsulation through localizing Changes
Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Files skipped from review as they are similar to previous changes (1)
Additional comments not posted (3)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
Actionable comments outside the diff hunks (2)
README.md (2)
Line range hint
24-35: Replace hard tabs with spaces for consistency and readability.- ```sh + ```sh - # Example using localhost:8080 + # Example using localhost:8080 - REACT_APP_BASE_URL=http://localhost:8080 npm start + REACT_APP_BASE_URL=http://localhost:8080 npm start
Line range hint
73-73: Remove multiple consecutive blank lines to maintain clean and professional formatting.- -
| const CardanoNS = window.cardano; | ||
|
|
There was a problem hiding this comment.
Tip
Codebase Verification
Review Summary
The review comment suggested that localizing the CardanoNS declaration within the connectWallet function in walletConnection.slice.ts was a good practice for encapsulation and reducing global namespace pollution. It also recommended verifying whether CardanoNS is used globally elsewhere in the codebase that might be affected by this change.
Verification Results
The executed script searched for occurrences of CardanoNS outside the walletConnection.slice.ts file. The results indicate that CardanoNS is indeed used in other parts of the codebase, specifically within ConnectSection.tsx. This file uses CardanoNS to check for installed wallet extensions and to display wallet-related information.
Conclusion
Given that CardanoNS is used in multiple parts of the application, the change to localize its declaration within connectWallet function might lead to issues if other parts of the application rely on a globally accessible CardanoNS. This could potentially break functionality in the ConnectSection.tsx where CardanoNS is expected to be globally accessible.
Action Required:
It is recommended to ensure that the changes in walletConnection.slice.ts do not inadvertently affect other components that rely on CardanoNS. A possible solution could be to maintain CardanoNS as a global variable or ensure that all components have access to it as needed.
Analysis chain
Localizing the CardanoNS declaration within the connectWallet function is a good practice for encapsulation and reducing global namespace pollution.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that `CardanoNS` is not used globally elsewhere in the codebase that might be affected by this change.
rg --type ts "CardanoNS" --glob '!src/store/slices/walletConnection.slice.ts'
Length of output: 762
| }); | ||
|
|
||
| export const startListenWalletChanges = createAsyncThunk('listenWalletChanges', async (payload: any, { dispatch, getState }) => { | ||
| const CardanoNS = window.cardano; |
There was a problem hiding this comment.
Tip
Codebase Verification
Review Summary
The review comment approved the localization of the CardanoNS declaration within the startListenWalletChanges function in walletConnection.slice.ts, suggesting that it aligns with good practices for reducing global namespace pollution. The review also included a verification script to check if CardanoNS is used globally elsewhere in the codebase that might be affected by this change.
Verification Findings
The script output indicates that CardanoNS is indeed used in other parts of the codebase, specifically within ConnectSection.tsx. This file contains multiple references to CardanoNS, which is used to check for the presence of wallet extensions and to display wallet-related information. This suggests that CardanoNS is not exclusively local to walletConnection.slice.ts and is required globally.
Conclusion
Given the usage of CardanoNS in multiple components outside of walletConnection.slice.ts, the localization of CardanoNS within startListenWalletChanges might lead to issues if other parts of the application rely on a globally accessible CardanoNS. This could potentially break functionality in ConnectSection.tsx where CardanoNS is expected to be available globally.
Action Required:
The localization of CardanoNS should be reconsidered, or adjustments should be made to ensure that all components that require CardanoNS have access to it. It is recommended to either maintain CardanoNS as a global variable or ensure that it is passed appropriately to all components that depend on it.
Analysis chain
Localizing the CardanoNS declaration within the startListenWalletChanges function is consistent with good practices for reducing global namespace pollution.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that `CardanoNS` is not used globally elsewhere in the codebase that might be affected by this change.
rg --type ts "CardanoNS" --glob '!src/store/slices/walletConnection.slice.ts'
Length of output: 762
There was a problem hiding this comment.
Actionable comments posted: 1
Actionable comments outside the diff hunks (2)
README.md (2)
Line range hint
20-27: Replace hard tabs with spaces to maintain consistency in the document formatting.- ```sh - # Example using localhost:8080 - REACT_APP_BASE_URL=http://localhost:8080 npm start - ``` + ```sh + # Example using localhost:8080 + REACT_APP_BASE_URL=http://localhost:8080 npm start + ```Also, consider adding a comma after
.envfor better readability.- The default API endpoint is `https://dapps-certification.scdev.aws.iohkdev.io/` found in `./.env` + The default API endpoint is `https://dapps-certification.scdev.aws.iohkdev.io/`, found in `./.env`
Line range hint
59-73: Reduce multiple consecutive blank lines to a single blank line for cleaner formatting.-# run the build -serve -s build - - -## Production docker + # run the build + serve -s build + + ## Production dockerAlso, consider adding a comma after 'docker' in the introductory note for clarity.
- NOTE: from the root of the project run all the docker-files scripts + NOTE: from the root of the project, run all the docker-files scripts
| | Statements | Branches | Functions | Lines | | ||
| | --------------------------- | ----------------------- | ------------------------- | ----------------- | | ||
| |  |  |  |  | | ||
| |  |  |  |  | |
There was a problem hiding this comment.
Clean up repeated whitespaces in the table headers for better readability.
-| Statements | Branches | Functions | Lines |
+| Statements | Branches | Functions | Lines |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.
| | Statements | Branches | Functions | Lines | | |
| | --------------------------- | ----------------------- | ------------------------- | ----------------- | | |
| |  |  |  |  | | |
| |  |  |  |  | | |
| | Statements | Branches | Functions | Lines | | |
| | --------------------------- | ----------------------- | ------------------------- | ----------------- | | |
| |  |  |  |  | |
Summary by CodeRabbit
New Features
RegisterModalcomponent to enhance user interaction on the landing page.ConnectSectionthat verifies modal functionality when the connect button is pressed.Bug Fixes
landing/index.tsxto avoid duplicate component loading.Documentation
Refactor
SubscriptionSection.CardanoNSusage within specific functions to improve code encapsulation.Tests
RegisterModal,RegisterSection, andSubscriptionSectionto ensure functionality and user interaction are as expected.Ref: PLT-9674
Checklist