Refactor FXIOS-12796 [Swift 6 Migration] Fix main actor isolation warnings that are Swift 6 errors in the XCUITests suite - Batch 2#28
Open
Refactor FXIOS-12796 [Swift 6 Migration] Fix main actor isolation warnings that are Swift 6 errors in the XCUITests suite - Batch 2#28
Conversation
Code Review by Qodo
1. A11yUtils.swift missing MPL header
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
1
to
4
| import XCTest | ||
|
|
||
| @MainActor | ||
| class A11yUtils: XCTestCase { |
There was a problem hiding this comment.
1. a11yutils.swift missing mpl header 📘 Rule violation ✓ Correctness
• The Swift file no longer begins with the required Mozilla Public License v2.0 header comment block. • This violates the licensing requirement for Swift source files and can create legal/compliance risk for redistributed source. • The file currently starts directly with import XCTest, indicating the header is absent from the top of the file.
Agent prompt
## Issue description
`A11yUtils.swift` is missing the required Mozilla Public License v2.0 header at the top of the file.
## Issue Context
Compliance requires every `.swift` file to start with the exact MPL v2.0 header text.
## Fix Focus Areas
- firefox-ios/firefox-ios-tests/Tests/XCUITests/A11yUtils.swift[1-6]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+25
to
26
| override func setUp() async throws { | ||
| // Test name looks like: "[Class testFunc]", parse out the function name |
There was a problem hiding this comment.
2. homepagesettingsuitest.swift bad suffix 📘 Rule violation ✓ Correctness
• The test file name HomePageSettingsUITest.swift does not follow the required *Test.swift / *Tests.swift naming convention. • This reduces discoverability/consistency and violates the enforced test file naming standard. • The file is actively modified in this PR (e.g., setUp signature), so it should be renamed to comply.
Agent prompt
## Issue description
The test file name does not comply with the required `*Test.swift` / `*Tests.swift` naming convention.
## Issue Context
The file is modified in this PR, so it should be renamed to meet the test naming standard and keep the suite consistent.
## Fix Focus Areas
- firefox-ios/firefox-ios-tests/Tests/XCUITests/HomePageSettingsUITest.swift[1-30]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+15
to
+22
| @MainActor | ||
| var goToCopiedLink = springboard.buttons["Go to Copied Link"] | ||
| @MainActor | ||
| var newPrivateSearch = springboard.buttons["New Private Search"] | ||
| @MainActor | ||
| var newSearch = springboard.buttons["New Search"] | ||
| @MainActor | ||
| var clearPrivateTabs = springboard.buttons["Clear Private Tabs"] |
There was a problem hiding this comment.
3. gotocopiedlink lacks access control 📘 Rule violation ✓ Correctness
• New/modified top-level properties (e.g., goToCopiedLink, newPrivateSearch, urlBarAddress) are declared without an explicit access control modifier. • This violates the requirement to declare explicit and minimal visibility (private/internal/public) and can unintentionally broaden API surface. • Add explicit access control appropriate to intended scope (likely private for file-local globals; internal if shared across tests).
Agent prompt
## Issue description
Several top-level properties/constants are declared with implicit access control, which violates the requirement for explicit and appropriate access modifiers.
## Issue Context
These globals were modified as part of Swift 6 concurrency changes (e.g., adding `@MainActor`), but still rely on implicit `internal` access.
## Fix Focus Areas
- firefox-ios/firefox-ios-tests/Tests/XCUITests/TodayWidgetTests.swift[14-42]
- firefox-ios/firefox-ios-tests/Tests/XCUITests/BaseTestCase.swift[10-16]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+508
to
512
| override func setUp() async throws { | ||
| specificForPlatform = .pad | ||
| if iPad() { | ||
| super.setUp() | ||
| await super.setUp() | ||
| } |
There was a problem hiding this comment.
4. Missing try await 🐞 Bug ✓ Correctness
• IpadOnlyTestCase.setUp() calls await super.setUp() even though the superclass setUp() is async throws. • This is a compile-time error that will block building the UI test target.
Agent prompt
### Issue description
`IpadOnlyTestCase.setUp()` calls a throwing async method without `try`, which is a compile error.
### Issue Context
`BaseTestCase.setUp()` is `async throws`, so platform-specific subclasses must call it with `try await`.
### Fix Focus Areas
- firefox-ios/firefox-ios-tests/Tests/XCUITests/BaseTestCase.swift[507-523]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+112
to
122
| override func setUp() async throws { | ||
| try await super.setUp() | ||
| continueAfterFailure = false | ||
| setUpApp() | ||
| setUpScreenGraph() | ||
| } | ||
|
|
||
| override func tearDown() { | ||
| override func tearDown() async throws { | ||
| app.terminate() | ||
| super.tearDown() | ||
| try await super.tearDown() | ||
| } |
There was a problem hiding this comment.
5. Restart helper now wrong 🐞 Bug ⛯ Reliability
• BaseTestCase.setUp/tearDown were migrated to async throws, but forceRestartApp() still calls synchronous tearDown()/setUp(). • This likely calls XCTest’s synchronous lifecycle methods (not BaseTestCase’s async ones), so the app may not actually restart and the screen graph may not be rebuilt. • Tests relying on forceRestartApp() to validate persistence across restart become incorrect/flaky.
Agent prompt
### Issue description
`forceRestartApp()` is synchronous but `BaseTestCase` lifecycle is now `async throws`. The helper can no longer reliably restart the app / rebuild state.
### Issue Context
Tests use `forceRestartApp()` to verify persistence across restart.
### Fix Focus Areas
- firefox-ios/firefox-ios-tests/Tests/XCUITests/BaseTestCase.swift[112-122]
- firefox-ios/firefox-ios-tests/Tests/XCUITests/BaseTestCase.swift[139-142]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+119
to
122
| override func tearDown() async throws { | ||
| app.terminate() | ||
| super.tearDown() | ||
| try await super.tearDown() | ||
| } |
There was a problem hiding this comment.
6. Sync teardown skipped 🐞 Bug ⛯ Reliability
• Several BaseTestCase-derived test classes still override *synchronous* tearDown() and call super.tearDown() (sync). • With BaseTestCase now implementing tearDown() async throws, XCTest will use the async tear down path, so these synchronous overrides’ cleanup (orientation/theme reset, downloads cleanup) can be skipped. • This risks cross-test pollution and UI-test flakiness (e.g., device left in landscape).
Agent prompt
### Issue description
Some test classes still override synchronous `tearDown()`, which is now disconnected from the async teardown path.
### Issue Context
`BaseTestCase` teardown is now `async throws`. Subclasses should override the same signature to ensure cleanup runs.
### Fix Focus Areas
- firefox-ios/firefox-ios-tests/Tests/XCUITests/BaseTestCase.swift[119-122]
- firefox-ios/firefox-ios-tests/Tests/XCUITests/ToolbarMenuTests.swift[8-12]
- firefox-ios/firefox-ios-tests/Tests/XCUITests/SettingsTests.swift[10-19]
- firefox-ios/firefox-ios-tests/Tests/XCUITests/DownloadsTests.swift[18-41]
- firefox-ios/firefox-ios-tests/Tests/XCUITests/DragAndDropTests.swift[28-31]
- firefox-ios/firefox-ios-tests/Tests/XCUITests/SwipingTabsTests.swift[9-12]
- firefox-ios/firefox-ios-tests/Tests/XCUITests/HomeButtonTests.swift[8-11]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+46
to
52
| override func setUp() async throws { | ||
| continueAfterFailure = false | ||
| setUpExperimentVariables() | ||
| setUpApp() | ||
| setUpExperimentVariables() | ||
| setUpLaunchArguments() | ||
| setUpScreenGraph() | ||
| } |
There was a problem hiding this comment.
7. Feature suite order crash 🐞 Bug ✓ Correctness
• FeatureFlaggedTestSuite.setUp() now calls setUpApp() before setUpExperimentVariables(). • But setUpApp() immediately uses jsonFileName/featureName, which are String! and documented to be set inside setUpExperimentVariables(). • If this base class is used, it can crash with an unexpected nil unwrap during setup.
Agent prompt
### Issue description
`FeatureFlaggedTestSuite.setUp()` uses IUO properties before they are initialized.
### Issue Context
`jsonFileName` / `featureName` are expected to be set by `setUpExperimentVariables()`.
### Fix Focus Areas
- firefox-ios/firefox-ios-tests/Tests/XCUITests/FeatureFlaggedTestBase.swift[37-52]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Benchmark PR from agentic-review-benchmarks#2