-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor FXIOS-14344 [Swift 6 migration] Fixing warnings in unit tests #32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: coderabbit_full_base_refactor_fxios-14344_swift_6_migration_fixing_warnings_in_unit_tests__pr6
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,9 +23,9 @@ class NotificationManagerTests: XCTestCase { | |
| } | ||
|
|
||
| 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") | ||
|
Comment on lines
+26
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: cat -n firefox-ios/firefox-ios-tests/Tests/ClientTests/Helpers/NotificationManagerTests.swiftRepository: qodo-benchmark/firefox-ios-combined-coderabbit Length of output: 5301 🌐 Web query:
💡 Result: Best practices (Swift 6 + strict concurrency) for XCTest assertions with
|
||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🌐 Web query:
XCTestCase async tearDown exception handling best practices💡 Result:
Best practices for
XCTestCaseasynctearDown+ exception/error handling1) In
tearDown() async throws, only do work that is actually awaitedIf you schedule work with
DispatchQueue.async/asyncAfter, XCTest won’t wait for it (because those APIs aren’tasync), so cleanup may never run. Preferawait-based APIs (or wrap callback-based APIs with continuations). (stackoverflow.com)2) Prefer teardown blocks (
addTeardownBlock) for per-test cleanup (especially when partially-initialized)Register cleanup right after you allocate/create a resource, so it gets cleaned up even if the test fails midway.
Notes from practice/documentation summaries:
tearDownmethodsawaitandthrowsetUp, but not fromtearDown(useyourloaf.com)3) Decide whether teardown errors should fail the test or be best-effort
tearDownWithError/tearDown() async throws. (Throwing from these methods is treated as a test failure.) (useyourloaf.com)4) Avoid
continueAfterFailure = falsewithasynctests / asyncsetUp/ asynctearDowncontinueAfterFailure = falseis implemented via Objective‑C exceptions; exceptions through Swiftasyncframes are problematic/undefined, and newer Xcode versions changed behavior to avoid reliability issues. Practically: keepcontinueAfterFailureat the defaulttrue, and usethrows/early-return patterns to stop the test. (gist.github.com)5) Don’t start “detached” tasks in teardown unless you intentionally don’t care if they finish
If you do
Task { ... }in teardown and don’tawaitit, the test can end before it completes, and the task may be cancelled or never observed. Prefer structured concurrency (await the work in teardown / teardown block). (stackoverflow.com)Citations:
🌐 Web query:
Swift XCTest tearDown super call order cleanup💡 Result:
In XCTest, the conventional (and safest) pattern is:
setUp…: callsuperfirst, then do your per-test setup.tearDown…: do your cleanup first, then callsuperlast. This matches Apple’s long-standing example (tearDownends with[super tearDown]). [1]Why this order matters / what runs when:
addTeardownBlock { … }blocks run after the test method ends and before the test case’s teardown methods. Multiple teardown blocks run LIFO (last registered, first executed). [2]setUp() async throws→setUpWithError()→setUp()tearDown()→tearDownWithError()→tearDown() async throws(and teardown blocks occur before these). [3]Do you need to call
super?XCTestCase, the base implementations are effectively “template methods” and are often empty, so omittingsuperusually doesn’t change behavior today—but callingsuperkeeps things correct if you later introduce a shared test base class or a framework superclass adds behavior. [4]Typical Swift shape
Sources: Apple archived testing guide example [1], teardown-block ordering notes [2], lifecycle ordering summary [3], discussion of why super calls are optional-but-wise [4].
[1] https://developer.apple.com/library/archive/documentation/DeveloperTools/Conceptual/testing_with_xcode/chapters/04-writing_tests.html
[2] https://useyourloaf.com/blog/unit-test-setup-and-teardown/
[3] https://useyourloaf.com/blog/unit-test-setup-and-teardown/
[4] https://stackoverflow.com/questions/53448538/xcode-10-and-super-teardown
Move cleanup before
super.tearDown()to ensure it runs even ifsuperthrows.XCTest's conventional pattern places cleanup before the
supercall intearDownmethods. Ifsuper.tearDown()throws an exception, the subsequent cleanup (nil assignments andDependencyHelperMock().reset()) won't execute, leaking state into later tests.♻️ Proposed fix
override func tearDown() async throws { - try await super.tearDown() diffableDataSource = nil collectionView = nil DependencyHelperMock().reset() + try await super.tearDown() }🤖 Prompt for AI Agents