-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[shared_preferences_foundation] Migrate XCTest to Swift Testing #10766
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: main
Are you sure you want to change the base?
Conversation
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.
Code Review
This pull request successfully migrates the tests for shared_preferences_foundation from XCTest to the modern Swift Testing framework. The changes are well-executed, utilizing parameterized tests to reduce boilerplate and improving test robustness by avoiding force unwraps. The decision to serialize tests is a good catch for the non-thread-safe API. I have one minor suggestion to improve consistency.
packages/shared_preferences/shared_preferences_foundation/darwin/Tests/RunnerTests.swift
Outdated
Show resolved
Hide resolved
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.
Xcode is expecting this directory:

macOS gets away with it because there's a (probably unnecessary) Info.plist in the equivalent directory:
https://github.com/flutter/packages/tree/fb3e9081ca58b1cb3e54b8bd471c57116a72585e/packages/shared_preferences/shared_preferences_foundation/example/macos/RunnerTests
| #endif | ||
|
|
||
| class RunnerTests: XCTestCase { | ||
| @Suite(.serialized) |
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.
Lots of test failures when these tests run in parallel. For example, there's no locking in the update methods like:
Lines 133 to 144 in fb3e908
| func clear(allowList: [String]?, options: SharedPreferencesPigeonOptions) throws { | |
| let defaults = try SharedPreferencesPlugin.getUserDefaults(options: options) | |
| if let allowList = allowList { | |
| for (key) in allowList { | |
| defaults.removeObject(forKey: key) | |
| } | |
| } else { | |
| for key in defaults.dictionaryRepresentation().keys { | |
| defaults.removeObject(forKey: key) | |
| } | |
| } | |
| } |
Should probably be migrated to actor?
Surprisingly I don't see any open issues that look related.
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.
Should probably be migrated to actor?
We now have toooo many options!
- Actor
- GCD
- the new Synchronization framework introduced last year
- Locks (
os_unfair_lockis likely the most performant)
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.
Worth adding a TODO to remove .serialized?
| #endif | ||
|
|
||
| class RunnerTests: XCTestCase { | ||
| @Suite(.serialized) |
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.
Should probably be migrated to actor?
We now have toooo many options!
- Actor
- GCD
- the new Synchronization framework introduced last year
- Locks (
os_unfair_lockis likely the most performant)
| #endif | ||
|
|
||
| class RunnerTests: XCTestCase { | ||
| @Suite(.serialized) |
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.
Worth adding a TODO to remove .serialized?
Part of flutter/flutter#180787
Use parameterized tests instead of testing in a for loop. I had to make the tests serializable because this shared preferences API is very un-threadsafe, and running them in parallel has many test failures where values are stomping on each other.
macOS tests
``` ◇ Test run started. ↳ Testing Library Version: 102 (arm64e-apple-macos13.0) ◇ Suite RunnerTests started. ◇ Test setAndGet(prefix:) started. ◇ Passing 1 argument prefix → "aPrefix" to setAndGet(prefix:) ◇ Passing 1 argument prefix → "" to setAndGet(prefix:) ✔ Test setAndGet(prefix:) passed after 0.001 seconds. ◇ Test getWithAllowList(prefix:) started. ◇ Passing 1 argument prefix → "aPrefix" to getWithAllowList(prefix:) ◇ Passing 1 argument prefix → "" to getWithAllowList(prefix:) ✔ Test getWithAllowList(prefix:) passed after 0.001 seconds. ◇ Test remove(prefix:) started. ◇ Passing 1 argument prefix → "aPrefix" to remove(prefix:) ◇ Passing 1 argument prefix → "" to remove(prefix:) ✔ Test remove(prefix:) passed after 0.030 seconds. ◇ Test clearWithNoAllowlist(prefix:) started. ◇ Passing 1 argument prefix → "aPrefix" to clearWithNoAllowlist(prefix:) ◇ Passing 1 argument prefix → "" to clearWithNoAllowlist(prefix:) ✔ Test clearWithNoAllowlist(prefix:) passed after 0.002 seconds. ◇ Test clearWithAllowlist(prefix:) started. ◇ Passing 1 argument prefix → "aPrefix" to clearWithAllowlist(prefix:) ◇ Passing 1 argument prefix → "" to clearWithAllowlist(prefix:) ✔ Test clearWithAllowlist(prefix:) passed after 0.001 seconds. ◇ Test asyncSetAndGet() started. ✔ Test asyncSetAndGet() passed after 0.001 seconds. ◇ Test asyncGetAll() started. ✔ Test asyncGetAll() passed after 0.001 seconds. ◇ Test asyncGetAllWithAndWithoutSuiteName() started. ✔ Test asyncGetAllWithAndWithoutSuiteName() passed after 0.001 seconds. ◇ Test asyncGetAllWithAllowList() started. ✔ Test asyncGetAllWithAllowList() passed after 0.001 seconds. ◇ Test asyncRemove() started. ✔ Test asyncRemove() passed after 0.001 seconds. ◇ Test asyncClearWithNoAllowlist() started. ✔ Test asyncClearWithNoAllowlist() passed after 0.001 seconds. ◇ Test asyncClearWithAllowlist() started. ✔ Test asyncClearWithAllowlist() passed after 0.001 seconds. ✔ Suite RunnerTests passed after 0.044 seconds. ✔ Test run with 12 tests passed after 0.044 seconds. ``` https://ci.chromium.org/ui/p/flutter/builders/try/Mac_arm64%20macos_platform_tests%20master%20-%20packages/26300/overviewiOS tests
``` Testing started Test suite 'RunnerTests' started on 'Clone 1 of Flutter-iPhone - Runner (85304)' Test case 'RunnerTests/setAndGet(prefix:)' passed on 'Clone 1 of Flutter-iPhone - Runner (85304)' (0.003 seconds) Test case 'RunnerTests/setAndGet(prefix:)' passed on 'Clone 1 of Flutter-iPhone - Runner (85304)' (0.002 seconds) Test case 'RunnerTests/getWithAllowList(prefix:)' passed on 'Clone 1 of Flutter-iPhone - Runner (85304)' (0.000 seconds) Test case 'RunnerTests/getWithAllowList(prefix:)' passed on 'Clone 1 of Flutter-iPhone - Runner (85304)' (0.000 seconds) Test case 'RunnerTests/remove(prefix:)' passed on 'Clone 1 of Flutter-iPhone - Runner (85304)' (0.001 seconds) Test case 'RunnerTests/remove(prefix:)' passed on 'Clone 1 of Flutter-iPhone - Runner (85304)' (0.001 seconds) Test case 'RunnerTests/clearWithNoAllowlist(prefix:)' passed on 'Clone 1 of Flutter-iPhone - Runner (85304)' (0.003 seconds) Test case 'RunnerTests/clearWithNoAllowlist(prefix:)' passed on 'Clone 1 of Flutter-iPhone - Runner (85304)' (0.013 seconds) Test case 'RunnerTests/clearWithAllowlist(prefix:)' passed on 'Clone 1 of Flutter-iPhone - Runner (85304)' (0.000 seconds) Test case 'RunnerTests/clearWithAllowlist(prefix:)' passed on 'Clone 1 of Flutter-iPhone - Runner (85304)' (0.000 seconds) Test case 'RunnerTests/asyncSetAndGet()' passed on 'Clone 1 of Flutter-iPhone - Runner (85304)' (0.001 seconds) Test case 'RunnerTests/asyncGetAll()' passed on 'Clone 1 of Flutter-iPhone - Runner (85304)' (0.000 seconds) Test case 'RunnerTests/asyncGetAllWithAndWithoutSuiteName()' passed on 'Clone 1 of Flutter-iPhone - Runner (85304)' (0.002 seconds) Test case 'RunnerTests/asyncGetAllWithAllowList()' passed on 'Clone 1 of Flutter-iPhone - Runner (85304)' (0.019 seconds) Test case 'RunnerTests/asyncRemove()' passed on 'Clone 1 of Flutter-iPhone - Runner (85304)' (0.001 seconds) Test case 'RunnerTests/asyncClearWithNoAllowlist()' passed on 'Clone 1 of Flutter-iPhone - Runner (85304)' (0.005 seconds) Test case 'RunnerTests/asyncClearWithAllowlist()' passed on 'Clone 1 of Flutter-iPhone - Runner (85304)' (0.002 seconds) Successfully ran iOS xctest for packages/shared_preferences/shared_preferences_foundation/example ``` https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8693085104608886001/+/u/Run_package_tests/native_test/stdout[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3