Refactor FXIOS-12796 [Swift 6 Migration] Fix main actor isolation warnings that are Swift 6 errors in the XCUITests suite - Batch 2#2
Refactor FXIOS-12796 [Swift 6 Migration] Fix main actor isolation warnings that are Swift 6 errors in the XCUITests suite - Batch 2#2
Conversation
📝 WalkthroughWalkthroughThis PR systematically refactors the Firefox iOS test suite to adopt async/throws semantics for lifecycle methods and enforce main-thread safety through Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 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
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
firefox-ios/firefox-ios-tests/Tests/XCUITests/BaseTestCase.swift (1)
139-142:⚠️ Potential issue | 🔴 CriticalMake
forceRestartApp()async/throws to call async setUp/tearDown.
setUp()andtearDown()are declared asasync throws(lines 112 and 119), so calling them from a sync method is a compile error and drops error handling.Proposed fix
- func forceRestartApp() { - tearDown() - setUp() - } + func forceRestartApp() async throws { + try await tearDown() + try await setUp() + }firefox-ios/firefox-ios-tests/Tests/XCUITests/TodayWidgetTests.swift (1)
60-72:⚠️ Potential issue | 🟠 MajorBound the swipe loop to avoid hangs.
If the expected element never appears, this loop can run indefinitely and stall CI.
🛠️ Suggested guard against infinite swipes
private func goToTodayWidgetPage() { + let maxSwipes = 5 + var swipeCount = 0 // Swipe right until the "Screen Time" icon appears if `#unavailable`(iOS 16) { - while !springboard.textFields["SpotlightSearchField"].exists { + while !springboard.textFields["SpotlightSearchField"].exists && swipeCount < maxSwipes { springboard.swipeRight() + swipeCount += 1 } + XCTAssertTrue( + springboard.textFields["SpotlightSearchField"].exists, + "Failed to reach Today Widget page within \(maxSwipes) swipes." + ) } else { - while !springboard.icons["Screen Time"].exists { + while !springboard.icons["Screen Time"].exists && swipeCount < maxSwipes { springboard.swipeRight() + swipeCount += 1 } + XCTAssertTrue( + springboard.icons["Screen Time"].exists, + "Failed to reach Today Widget page within \(maxSwipes) swipes." + ) } }
🤖 Fix all issues with AI agents
In `@firefox-ios/firefox-ios-tests/Tests/XCUITests/BaseTestCase.swift`:
- Around line 508-512: The call to super.setUp() inside BaseTestCase.setUp() is
missing the required try when awaiting an async throws method; update the
conditional branch where iPad() is true to call try await super.setUp()
(matching the pattern used in IphoneOnlyTestCase.setUp()) so the throwable async
initializer is properly propagated.
| override func setUp() async throws { | ||
| specificForPlatform = .pad | ||
| if iPad() { | ||
| super.setUp() | ||
| await super.setUp() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "BaseTestCase.swift" -type fRepository: agentic-review-benchmarks/firefox-ios-combined-coderabbit
Length of output: 223
🏁 Script executed:
cat -n ./firefox-ios/firefox-ios-tests/Tests/XCUITests/BaseTestCase.swift | sed -n '500,520p'Repository: agentic-review-benchmarks/firefox-ios-combined-coderabbit
Length of output: 797
🏁 Script executed:
cat -n ./firefox-ios/firefox-ios-tests/Tests/XCUITests/BaseTestCase.swift | sed -n '1,100p'Repository: agentic-review-benchmarks/firefox-ios-combined-coderabbit
Length of output: 5195
🏁 Script executed:
cat -n ./firefox-ios/firefox-ios-tests/Tests/XCUITests/BaseTestCase.swift | sed -n '100,200p'Repository: agentic-review-benchmarks/firefox-ios-combined-coderabbit
Length of output: 4097
🏁 Script executed:
cat -n ./firefox-ios/firefox-ios-tests/Tests/XCUITests/BaseTestCase.swift | sed -n '507,525p'Repository: agentic-review-benchmarks/firefox-ios-combined-coderabbit
Length of output: 639
Add try when awaiting super.setUp() in iPad-only setup.
BaseTestCase.setUp() is async throws, so this call must be try await. See IphoneOnlyTestCase.setUp() at line 520 for the correct pattern.
Proposed fix
- if iPad() {
- await super.setUp()
- }
+ if iPad() {
+ try await super.setUp()
+ }🤖 Prompt for AI Agents
In `@firefox-ios/firefox-ios-tests/Tests/XCUITests/BaseTestCase.swift` around
lines 508 - 512, The call to super.setUp() inside BaseTestCase.setUp() is
missing the required try when awaiting an async throws method; update the
conditional branch where iPad() is true to call try await super.setUp()
(matching the pattern used in IphoneOnlyTestCase.setUp()) so the throwable async
initializer is properly propagated.
Benchmark PR from agentic-review-benchmarks#2
Summary by CodeRabbit