Refactor FXIOS-14344 [Swift 6 migration] Fixing warnings in unit tests #19
Conversation
WalkthroughThis PR systematically converts test lifecycle methods from synchronous to asynchronous throwing variants across multiple test classes, updates setUp and tearDown method signatures to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/StoriesFeed/StoriesFeedDiffableDataSourceTests.swift`:
- Around line 28-33: The tearDown method calls try await super.tearDown() too
early; move the super.tearDown() call to the end of tearDown so test-specific
cleanup runs first: nil out diffableDataSource and collectionView and call
DependencyHelperMock().reset() before invoking try await super.tearDown() in the
override of func tearDown() async throws (method name: tearDown; symbols:
diffableDataSource, collectionView, DependencyHelperMock().reset()).
In
`@firefox-ios/firefox-ios-tests/Tests/ClientTests/Helpers/NotificationManagerTests.swift`:
- Around line 25-30: Replace the synchronous assert() calls in
testRequestAuthorization with XCTest assertions and an expectation: use an
XCTestExpectation, call notificationManager.requestAuthorization and in its
completion fulfill the expectation and use XCTAssertTrue for granted and
XCTAssertTrue(center?.requestAuthorizationWasCalled ?? false) (or XCTAssertEqual
as appropriate), then waitForExpectations with a timeout; this ensures
testRequestAuthorization waits for the asynchronous completion and reports
failures via XCTest instead of crashing.
🧹 Nitpick comments (6)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockBrowserViewControllerWebViewDelegates.swift (1)
23-45: Consider addingfinaltoMockNavigationActionfor consistency.This mock class is the only one in the file not marked as
final. For consistency withMockFrameInfo,MockURLAuthenticationChallengeSender, andMockFileManager, consider making this classfinalas well.♻️ Suggested change
-class MockNavigationAction: WKNavigationAction { +final class MockNavigationAction: WKNavigationAction {firefox-ios/firefox-ios-tests/Tests/ClientTests/NativeErrorPage/NativeErrorPageViewControllerTests.swift (1)
19-22: Consider reordering tearDown: reset dependencies before calling super.The current order calls
super.tearDown()beforeDependencyHelperMock().reset(). Standard tearDown practice is to clean up local/test state first, then call the superclass method. This is the inverse of setUp order.Suggested fix
override func tearDown() { - super.tearDown() DependencyHelperMock().reset() + super.tearDown() }firefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuCoordinatorTests.swift (1)
20-23: Consider settingmockRouterto nil in tearDown for test isolation.The
mockRouterproperty is initialized insetUpbut not cleaned up intearDown. While this may work in practice since a new instance is created in eachsetUp, explicitly nilling it out ensures proper test isolation and matches the pattern used in other test files in this PR.Suggested fix
override func tearDown() async throws { DependencyHelperMock().reset() + mockRouter = nil try await super.tearDown() }firefox-ios/firefox-ios-tests/Tests/ClientTests/OnboardingTests/IntroViewControllerTests.swift (2)
20-23: MissingDependencyHelperMock().reset()in tearDown.The
setUpcallsDependencyHelperMock().bootstrapDependencies(), buttearDowndoesn't callreset(). Other test files in this PR (e.g.,MainMenuCoordinatorTests,SyncContentSettingsViewControllerTests) consistently reset dependencies in tearDown. This could affect test isolation.Suggested fix
override func tearDown() async throws { + DependencyHelperMock().reset() mockNotificationCenter = nil try await super.tearDown() }
77-78: Redundant@MainActorannotation.This method-level
@MainActorannotation is redundant since the class is already annotated with@MainActorat line 10. The method will inherit the actor isolation from the class.Suggested fix
- `@MainActor` func testViewMovesToNextScreenAfterNotification() {firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/StoriesFeed/StoriesFeedDiffableDataSourceTests.swift (1)
44-45: Redundant@MainActorannotation.This method-level
@MainActorannotation is redundant since the class is already annotated with@MainActorat line 11. The method inherits actor isolation from the class.Suggested fix
- `@MainActor` func test_updateSnapshot_withValidState_returnsStories() throws {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/StoriesFeed/StoriesFeedDiffableDataSourceTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/SyncContentSettingsViewControllerTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/FxAWebViewModelTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Helpers/NotificationManagerTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Helpers/RatingPromptManagerTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Library/Bookmarks/EditBookmarkDataSourceTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Library/Bookmarks/EditBookmarkViewModelTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Library/HistoryPanelTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuConfigurationUtilityTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuCoordinatorTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuMiddlewareTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuViewControllerTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyCoordinatorTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyMiddlewareTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyViewControllerTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockBrowserViewControllerWebViewDelegates.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockProfile.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockUserNotificationCenter.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/NativeErrorPage/NativeErrorPageViewControllerTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/OnboardingTests/IntroViewControllerTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/OnboardingTests/IntroViewModelTests.swift
🔇 Additional comments (18)
firefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuMiddlewareTests.swift (2)
15-20: LGTM!The conversion to
async throwswithtry await super.setUp()called first follows the correct XCTest lifecycle pattern for Swift 6 migration. Parent setup executes before child initialization.
22-27: LGTM!The tearDown order is correct: child cleanup (reset dependencies, nil out mocks, reset store) happens before calling
try await super.tearDown(). This aligns with the commit message "Fix teardown methods orders" and follows the proper pattern of reversing the setup sequence.firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockBrowserViewControllerWebViewDelegates.swift (1)
48-54: LGTM! Addingfinalis appropriate for this mock class.The change aligns well with Swift 6 migration goals and is consistent with other mocks in this file (
MockFrameInfo,MockFileManager).firefox-ios/firefox-ios-tests/Tests/ClientTests/OnboardingTests/IntroViewModelTests.swift (1)
16-25: LGTM!The migration to
async throwslifecycle methods is correct. The tearDown properly nullifiesnimbusUtilitybefore callingsuper.tearDown(), which follows the recommended cleanup order.firefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuViewControllerTests.swift (1)
10-22: LGTM on the@MainActorannotation.Adding
@MainActoris appropriate sinceMainMenuViewControlleris a UIKit view controller requiring main thread access.Note: Unlike other test files in this PR (e.g.,
MainMenuConfigurationUtilityTests), thesetUp/tearDownmethods here remain synchronous. This works fine but creates slight inconsistency across the test suite. Consider aligning with the async pattern if desired.firefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuConfigurationUtilityTests.swift (1)
17-27: LGTM!The migration to
async throwslifecycle methods is correct. The tearDown properly cleans up dependencies and nullifies the utility before callingsuper.tearDown().firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockUserNotificationCenter.swift (1)
9-9: Pragmatic choice for test code, but note the implications.Adding
@unchecked Sendablesuppresses concurrency warnings but the class has unsynchronized mutable state (pendingRequests,*WasCalledflags). This is acceptable for test mocks that run sequentially, but be aware this asserts thread-safety that isn't actually enforced.If tests ever run concurrently or the mock is captured across actor boundaries with concurrent access, data races could occur.
firefox-ios/firefox-ios-tests/Tests/ClientTests/Library/Bookmarks/EditBookmarkDataSourceTests.swift (1)
8-18: Swift 6 XCTest lifecycle migration looks good.
@MainActorplus async/throws setup/teardown with awaited super calls is consistent with the rest of the migration.Also applies to: 25-28
firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockProfile.swift (1)
93-97: Good concurrency annotation update.Adding
@MainActorand@Sendableto the completion keeps the API aligned with actor isolation expectations.firefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyViewControllerTests.swift (1)
10-11: Main-actor isolation for UI tests is appropriate.firefox-ios/firefox-ios-tests/Tests/ClientTests/Library/HistoryPanelTests.swift (1)
14-16: Async XCTest lifecycle update looks correct.Also applies to: 21-25
firefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyCoordinatorTests.swift (1)
15-16: Async setup/teardown conversion is solid.Also applies to: 22-25
firefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyMiddlewareTests.swift (1)
15-16: Async lifecycle migration is consistent and clean.Also applies to: 22-27
firefox-ios/firefox-ios-tests/Tests/ClientTests/Library/Bookmarks/EditBookmarkViewModelTests.swift (1)
33-45: LGTM! Async lifecycle methods are correctly implemented.The
setUpcorrectly callssuper.setUp()first before initializing test fixtures, andtearDownproperly cleans up test-specific resources before callingsuper.tearDown()last. This follows the correct order for XCTest lifecycle methods.firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/SyncContentSettingsViewControllerTests.swift (1)
15-29: LGTM! Async lifecycle methods are correctly implemented.The lifecycle methods follow the correct pattern:
setUpcalls super first then initializes test fixtures, andtearDowncleans up dependencies and test objects before calling super last.firefox-ios/firefox-ios-tests/Tests/ClientTests/FxAWebViewModelTests.swift (1)
14-27: LGTM! Async lifecycle methods are correctly implemented.The
setUpandtearDownmethods follow the correct ordering pattern for XCTest async lifecycle methods, with proper resource initialization and cleanup.firefox-ios/firefox-ios-tests/Tests/ClientTests/Helpers/RatingPromptManagerTests.swift (2)
22-24: AsyncsetUpmigration looks correct.
Awaitingsuper.setUp()before initializing fixtures is appropriate.
36-45: AsynctearDownmigration looks correct.
Usingtry await super.tearDown()in the async override is consistent with XCTest’s async lifecycle.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| override func tearDown() async throws { | ||
| try await super.tearDown() | ||
| diffableDataSource = nil | ||
| collectionView = nil | ||
| DependencyHelperMock().reset() | ||
| super.tearDown() | ||
| } |
There was a problem hiding this comment.
Incorrect tearDown order: super.tearDown() should be called last.
The super.tearDown() call on line 29 should be the last statement in the method, not the first. Test-specific cleanup (nilling out diffableDataSource, collectionView, and resetting dependencies) should happen before calling the superclass tearDown. This is the opposite order of setUp where super is called first.
The other test files in this PR correctly place super.tearDown() at the end.
Proposed fix
override func tearDown() async throws {
- try await super.tearDown()
diffableDataSource = nil
collectionView = nil
DependencyHelperMock().reset()
+ try await super.tearDown()
}📝 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.
| override func tearDown() async throws { | |
| try await super.tearDown() | |
| diffableDataSource = nil | |
| collectionView = nil | |
| DependencyHelperMock().reset() | |
| super.tearDown() | |
| } | |
| override func tearDown() async throws { | |
| diffableDataSource = nil | |
| collectionView = nil | |
| DependencyHelperMock().reset() | |
| try await super.tearDown() | |
| } |
🤖 Prompt for AI Agents
In
`@firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/StoriesFeed/StoriesFeedDiffableDataSourceTests.swift`
around lines 28 - 33, The tearDown method calls try await super.tearDown() too
early; move the super.tearDown() call to the end of tearDown so test-specific
cleanup runs first: nil out diffableDataSource and collectionView and call
DependencyHelperMock().reset() before invoking try await super.tearDown() in the
override of func tearDown() async throws (method name: tearDown; symbols:
diffableDataSource, collectionView, DependencyHelperMock().reset()).
| func testRequestAuthorization() { | ||
| notificationManager.requestAuthorization { (granted, error) in | ||
| XCTAssertTrue(granted) | ||
| XCTAssertTrue(self.center.requestAuthorizationWasCalled) | ||
| notificationManager.requestAuthorization { [center] (granted, error) in | ||
| assert(granted, "Authorization should be granted") | ||
| assert(center?.requestAuthorizationWasCalled ?? false, "requestAuthorization should be called") | ||
| } | ||
| } |
There was a problem hiding this comment.
Using assert() instead of XCTAssert* will not report test failures correctly.
The change from XCTAssertTrue to assert() is problematic:
assert()doesn't integrate with XCTest—failures won't be reported as test failures in test resultsassert()crashes the process rather than gracefully failing the test- The completion handler executes asynchronously but the test doesn't wait for it, so assertions may not run before the test completes
Suggested fix using expectations
func testRequestAuthorization() {
+ let expectation = expectation(description: "Authorization callback")
notificationManager.requestAuthorization { [center] (granted, error) in
- assert(granted, "Authorization should be granted")
- assert(center?.requestAuthorizationWasCalled ?? false, "requestAuthorization should be called")
+ XCTAssertTrue(granted, "Authorization should be granted")
+ XCTAssertTrue(center?.requestAuthorizationWasCalled ?? false, "requestAuthorization should be called")
+ expectation.fulfill()
}
+ wait(for: [expectation], timeout: 1.0)
}🤖 Prompt for AI Agents
In
`@firefox-ios/firefox-ios-tests/Tests/ClientTests/Helpers/NotificationManagerTests.swift`
around lines 25 - 30, Replace the synchronous assert() calls in
testRequestAuthorization with XCTest assertions and an expectation: use an
XCTestExpectation, call notificationManager.requestAuthorization and in its
completion fulfill the expectation and use XCTAssertTrue for granted and
XCTAssertTrue(center?.requestAuthorizationWasCalled ?? false) (or XCTAssertEqual
as appropriate), then waitForExpectations with a timeout; this ensures
testRequestAuthorization waits for the asynchronous completion and reports
failures via XCTest instead of crashing.
Benchmark PR from qodo-benchmark#236
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.