-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Member
Author
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. Xcode is expecting this directory: macOS gets away with it because there's a (probably unnecessary) Info.plist in the equivalent directory: |

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:
packages/packages/shared_preferences/shared_preferences_foundation/darwin/shared_preferences_foundation/Sources/shared_preferences_foundation/SharedPreferencesPlugin.swift
Lines 133 to 144 in fb3e908
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.
We now have toooo many options!
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?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.
Come to think of it, this isn't actually a bug. The defaults themselves are a threadsafe API, so if calling code, like these tests, are relying on another thread not mutating the defaults, then it would up to that calling code to handle it. I actually now think
@Suite(.serialized)is correct. I'll add a comment.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.
I was going to say something like this. My understanding when I wrote this code is that behavior is expected. I can see changing it though, if we wanted to make the plugin work that way instead.
Uh oh!
There was an error while loading. Please reload this page.
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.
There's still a few pieces here that look a little thread unsafe, for example:
packages/packages/shared_preferences/shared_preferences_foundation/darwin/shared_preferences_foundation/Sources/shared_preferences_foundation/SharedPreferencesPlugin.swift
Lines 140 to 142 in fb3e908
That's looping over a collection while mutating it, which is generally not a good pattern. There's no guarantee the defaults wouldn't be added by another thread while it's being cleared.correction: no it's not, it's looping over the dictionary representation.But it's way less dire than I made it out to be in the initial description. Sorry.
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.
Had me worried there for a second :)
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.
If there are others, I'd be happy to hear about them though. I'm always happy to learn from my mistakes.
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.
Sorry for the noise and panic, @tarrinneal 🙂