[shared_preferences] Fix possible clash of string with double entry #3895
[shared_preferences] Fix possible clash of string with double entry #3895
Conversation
1790f96
to
83c8243
Compare
Limitations in existing testing isn't a reason to not add new tests. Is there a reason this is not easily testable? It seems like a unit test calling the method handler and validating that |
Is testing of the Android side already available? If so, can you point me to how that's done? |
It looks like Android unit tests aren't set up for this plugin yet. Integration tests should be fine for this though. See https://github.com/flutter/flutter/wiki/Plugin-Tests for pointers to where the test files are and how to run them. |
This prevents overwriting a double entry with a string on Android
This adds integration tests for string clashes, but limits them to Android
4f61932
to
90a2604
Compare
Integration test for string clashes are added and succeed |
Resolved a merge conflict that was caused by a different PR/push adding an issue tracker to pubspec.yaml |
@@ -1,6 +1,7 @@ | |||
## NEXT |
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.
This needs to be updated to bump the version.
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.
Done in the new commit.
@@ -128,6 +128,15 @@ class SharedPreferences { | |||
_setValue('Double', key, value); | |||
|
|||
/// Saves a string [value] to persistent storage in the background. | |||
/// | |||
/// Note: Due to limitations in Android's SharedPreferences, | |||
/// values cannot start with either one of the following: |
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.
Nit: s/either one/any of/
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.
Done in the new commit.
.../shared_preferences/shared_preferences/example/integration_test/shared_preferences_test.dart
Show resolved
Hide resolved
packages/shared_preferences/shared_preferences/lib/shared_preferences.dart
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.
Looks good, but unfortunately you'll need to merge in the latest master again so that the CI checks will run, due to an infra issue that required updates to in-repo configuration to resolve.
9b4a77a
to
a0f3381
Compare
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.
LGTM, thanks!
This prevents clashing of a string entry with a double entry.
This was already checked for string lists and big integers, yet doubles remained previously unchecked although being stored as strings as well.
Pre-launch Checklist
dart format
. See plugin_tool format)[shared_preferences]
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.