test: fix failing tests due to outdated router spies #1004
Conversation
WalkthroughThe pull request includes modifications to the test suites of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
CodeRabbit Configuration File (
|
|
PR description must contain a link to a ClickUp (case-insensitive) |
![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
🧹 Outside diff range and nitpick comments (3)
src/app/integrations/qbo/qbo.component.spec.ts (2)
25-25: Improved test suite setup with modern Angular practices.The changes to the test suite setup are commendable:
- Using a real
Routerinstance instead of a spy object allows for more accurate testing of routing behavior.- Including
SharedModuleandHttpClientTestingModulein the imports enhances the test environment.- Using
provideRouter([])in the TestBed configuration is the correct way to set up routing in the test environment.These updates align with best practices in Angular testing.
Consider adding some mock routes to the
provideRouter([])call to more closely simulate the actual routing configuration of your application. For example:provideRouter([ { path: 'integrations/qbo/onboarding/landing', component: DummyComponent }, // Add other relevant routes ])This would allow for more comprehensive testing of navigation scenarios.
Also applies to: 50-51, 58-59
Line range hint
114-120: Improved test structure for onboarding states, but lacking assertions.The update to use a forEach loop for testing different onboarding states is a good improvement:
- It allows for more concise testing of multiple states.
- Makes it easier to add new states to test in the future.
However, there's an important issue to address:
The test case is missing expectations within the loop. Without assertions, the test isn't verifying any behavior for the different onboarding states.
Consider adding appropriate expectations within the loop. For example:
testOnboardingState.forEach(({ state, expectedRoute }) => { const testWorkspace = { ...mockWorkspace, onboarding_state: state }; workspaceServiceSpy.getWorkspace.and.returnValue(of([testWorkspace])); fixture.detectChanges(); tick(); expect(router.navigateByUrl).toHaveBeenCalledWith(expectedRoute); });This ensures that the correct navigation occurs for each onboarding state.
src/app/integrations/intacct/intacct.component.spec.ts (1)
63-63: LGTM: Router setup improved, with a minor suggestionThe changes to the router setup are good:
- Directly injecting the Router is cleaner than using a spy object.
- Spying on specific methods provides more precise control over the router behavior in tests.
- Using spyOnProperty for router.events is a good practice for mocking observables.
These changes align with best practices for Angular testing.
Consider moving the router spies to the beforeEach block for consistency with other service spies:
beforeEach(() => { // ... other setup code ... router = TestBed.inject(Router); spyOn(router, 'navigateByUrl'); spyOnProperty(router, 'events').and.returnValue(of(new NavigationEnd(0, '', ''))); });Also applies to: 66-67
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/app/integrations/intacct/intacct.component.spec.ts (7 hunks)
- src/app/integrations/qbo/qbo.component.spec.ts (6 hunks)
🧰 Additional context used
🔇 Additional comments (11)
src/app/integrations/qbo/qbo.component.spec.ts (4)
2-2: Improved import statements and test setup.The changes to the import statements are beneficial:
- Using
provideRouterinstead of directly importingRouteraligns with modern Angular testing practices.- Including
SharedModuleandHttpClientTestingModuleenhances the test environment, potentially providing necessary dependencies and HTTP testing utilities.These updates contribute to a more robust and maintainable test suite.
Also applies to: 13-14
68-69: Improved router testing setup.The changes to the router setup enhance the test suite:
- Injecting the
Routerdirectly instead of creating a spy object allows for more realistic testing scenarios.- Spying on the
navigateByUrlmethod of the injected router provides a way to verify navigation without mocking the entire Router service.This approach offers a good balance between isolation and integration testing for routing behavior.
97-97: Consistent update to navigation expectation.The change in the expect statement correctly reflects the earlier modifications:
- Using
router.navigateByUrlinstead ofrouterSpy.navigateByUrlis consistent with the new approach of using a real Router instance.- This ensures that the test accurately verifies the navigation behavior of the component.
110-110: Consistent update to another navigation expectation.This change maintains consistency with the new Router testing approach:
- Using
router.navigateByUrlinstead ofrouterSpy.navigateByUrlin this expect statement aligns with the earlier modifications.- It ensures that navigation behavior is consistently tested across different scenarios in the test suite.
src/app/integrations/intacct/intacct.component.spec.ts (7)
2-2: LGTM: Import changes enhance testing setupThe changes to the imports are appropriate:
- Replacing
RouterModulewithprovideRouteraligns with modern Angular testing practices.- Adding
SharedModuleandHttpClientTestingModuleenhances the testing environment with shared components and HTTP testing capabilities.These changes should improve the overall testing setup.
Also applies to: 14-15
46-47: LGTM: Test module setup improvedThe changes to the test module setup are appropriate:
- Separating the IntacctComponent declaration improves readability.
- Adding SharedModule and HttpClientTestingModule to imports is consistent with the new import statements.
- Using provideRouter in the providers array is the correct approach for the updated Angular testing practices.
These changes should result in a more robust and maintainable test environment.
Also applies to: 54-55
90-90: LGTM: Router assertion updated correctlyThe change to directly assert
router.navigateByUrlis consistent with the new router setup. This modification maintains the same test logic while aligning with the updated testing approach.
102-102: LGTM: Router assertion updated consistentlyThe change to directly assert
router.navigateByUrlis consistent with the new router setup and the previous test case. This modification maintains the same test logic while aligning with the updated testing approach.
106-106: Excellent improvement to test case structureGreat job on refactoring this test case! The changes bring several benefits:
- Using
Object.entriesto iterate over test cases makes the test more dynamic and maintainable.- It's now easier to add or modify test cases without changing the test structure.
- The assertion on
router.navigateByUrlis consistent with previous changes.This refactoring improves both the readability and maintainability of the test suite. Well done!
Also applies to: 112-112
123-123: LGTM: Router assertion updated consistentlyThe change to directly assert
router.navigateByUrlis consistent with the new router setup and previous test cases. This modification maintains the same test logic while aligning with the updated testing approach.
Line range hint
1-137: Overall: Excellent improvements to the test suiteThis pull request makes several positive changes to the IntacctComponent test suite:
- Updated imports and test module setup to use modern Angular testing practices.
- Improved router testing by directly injecting and spying on the Router.
- Refactored test cases for better readability and maintainability.
- Consistently updated router assertions across all test cases.
These changes align well with the PR objectives of fixing failing tests due to outdated router spies. The new approach should result in more robust and maintainable tests.
One minor suggestion was made to move the router spies to the beforeEach block for consistency, but this is a small optimization and not critical.
Great work on improving the test suite!
| @@ -125,6 +127,6 @@ describe('QboComponent', () => { | |||
| fixture.detectChanges(); | |||
| tick(); | |||
|
|
|||
| expect(routerSpy.navigateByUrl).toHaveBeenCalled(); | |||
| expect(router.navigateByUrl).toHaveBeenCalled(); | |||
There was a problem hiding this comment.
Consistent update to navigation expectation, but potential logical error in the test.
The change to use router.navigateByUrl instead of routerSpy.navigateByUrl is consistent with the earlier modifications and maintains the new approach of using a real Router instance.
However, there's a potential issue with the test logic:
The test description states "should not navigate if pathname is not /integrations/qbo", but the expectation expect(router.navigateByUrl).toHaveBeenCalled() checks if navigation did occur. This seems contradictory.
Consider updating the expectation to match the test description:
expect(router.navigateByUrl).not.toHaveBeenCalled();This would correctly verify that no navigation occurs when the pathname is not "/integrations/qbo".
JustARatherRidiculouslyLongUsername
merged commit 6d0540c
into
master
Description
We were using deprecated ways to mock and test routers in
IntacctComponentandQboComponentunit tests, which caused tests to fail. All tests are passing nowClickup
https://app.clickup.com/t/86cwabccw
https://app.clickup.com/t/86cwh86bz
Summary by CodeRabbit
Bug Fixes
IntacctComponentandQboComponentby streamlining router usage in tests.Tests
SharedModuleandHttpClientTestingModule.