[shared_preferences] Add shared preferences devtools

[adsonpleal]

This PR adds the shared_preferences_tools package. This package user the devtools_extension tooling to create a tool for shared preferences. The idea of this PR came from @kenzieschmoll on this issue. Initially I've published this tool as a separate package, but this PR aims to bring the functionality to the main shared_preferences package. Once this PR gets merged I'll archive the shared_preferences_tools package.

shared_preferences_tools.mp4

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I linked to at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[kenzieschmoll]

all the new dart files in this PR need the Flutter copyright:

// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

[adsonpleal]

@kenzieschmoll Done, all the requested changes were applied. One thing that I'd like to do, though, is some integrations tests. I was exploring the devtools github repo and saw that you already have a devtools_test package, but it doesn't have the fixture tooling used in the devtools/devtools package. The best scenario would be to be able to create fixture tests like this one.

Do you have plans of extracting this tooling to the devtools_test package? Or is there some docs on how to do fixture tests for devtools_extensions packages?

[kenzieschmoll]

How long does that build take?

@stuartmorgan depending on the size of the app, maybe a minute or two? The build command builds the extensions flutter web app in release mode, and then copies the assets to the extension/devtools/build folder.

[stuartmorgan]

Adding a minute or two to the publish step is fine. We'll need to add repo tooling for packages to be able to specify a pre-publish hook, probably via a tools/some_known_name.dart script that does whatever needs to be done.

[adsonpleal]

@kenzieschmoll and @stuartmorgan also the tests need to run with the --platform chrome flag since I'm doing some UI tests and the devtools_extensions won't run without this flag.

flutter test --platform chrome

[kenzieschmoll]

@kenzieschmoll Done, all the requested changes were applied. One thing that I'd like to do, though, is some integrations tests. I was exploring the devtools github repo and saw that you already have a devtools_test package, but it doesn't have the fixture tooling used in the devtools/devtools package. The best scenario would be to be able to create fixture tests like this one.

Do you have plans of extracting this tooling to the devtools_test package? Or is there some docs on how to do fixture tests for devtools_extensions packages?

We are not planning on publishing the devtools_test package, but devtools_shared does have a library devtools_test_utils.dart that should what you are looking for.

You can use the integration test of the simulated environment in package:devtools_extensions as a guide: https://github.com/flutter/devtools/tree/master/packages/devtools_extensions/integration_test.

[kenzieschmoll]

@stuartmorgan WRT to testing, is there a way to add an integration test hook to the CI? For example, the flutter/devtools CI has this code to run the flutter web integration tests for the devtools_extensions package: https://github.com/flutter/devtools/blob/master/tool/ci/bots.sh#L91-L94

[stuartmorgan]

@stuartmorgan WRT to testing, is there a way to add an integration test hook to the CI? For example, the flutter/devtools CI has this code to run the flutter web integration tests for the devtools_extensions package: https://github.com/flutter/devtools/blob/master/tool/ci/bots.sh#L91-L94

It's possible to run arbitrary bespoke tests with tool/run_tests.dart, but that's generally an approach of last resort. What specifically do the tests here need that isn't covered by standard integration test invocation?

[kenzieschmoll]

This looks great! Done with first round of review on the extension content itself.

[adsonpleal]

@kenzieschmoll and @stuartmorgan I can't make both Linux analyze stable and Linux analyze master pass. The flutter_test SDK package has the vm_service version pinned. And the version has changed in the master version. So, if I pin the version to ^14.3.0 it breaks on stable, if I change to ^14.2.5 it breaks on master, since I'm using mockito to mock the vm VmService class. This breaks the compilation, since ^14.3.0 changed the api and mockito is generating mocks for the old api.

I see two options here:

1 - Drop mockito altogether.
2 - Create my own VmService mock class, but keep mockito for the other.

WDYT?

I'll start with 2 and see if it fixes this issue.

[adsonpleal]

@kenzieschmoll this PR is ready for review again.

[kenzieschmoll]

can we rename this DevToolsExtensionDataProvider or SharedPreferencesDevToolsExtensionData or similar? This class name is very close to DevToolsExtension, which is the wrapper widget from package:devtools_extensions, which is a bit confusing.

[kenzieschmoll]

nit r/devtools/DevTools

[kenzieschmoll]

super-nit: period at the end of sentences

[kenzieschmoll]

can this be a const constructor?

[adsonpleal]

Good question, just followed @sigmundch's suggestion. I'll try the const constructor to see if the web runner keeps the library.

[adsonpleal]

It can be const, just changed it.

[kenzieschmoll]

nit: r/and event/an event. Also r/post/posts

[kenzieschmoll]

typos:

• remove first "a"
• r/give/given
• r/post/posts
• period at end of sentence instead of semi colon

[kenzieschmoll]

r/and/an

[adsonpleal]

dyslexia hitting hard 😬

[kenzieschmoll]

will value ever be null? if so, should we handle the null case instead of null asserting?

[adsonpleal]

Not in the way it is implemented. There is no way to set it as null. The only option is removing it, but this is another method.

[kenzieschmoll]

r/and/an

[kenzieschmoll]

nit: should this be shared_preferences. instead of shared_preferences:? Using dot is a more common convention for event names than using a colon.

[adsonpleal]

I think I copied this from somewhere, gonna change it to ..

[kenzieschmoll]

nit: r/post/posts

[kenzieschmoll]

for this test case and the test below, can you include what type the value is in the test name so that the tests are easy to differentiate (like you did for the async api test below)? Also, it looks like we are only testing bool and int values for the legacy API. Should we be testing every type like you do for the async api below?

[adsonpleal]

Since we're using the get method in the requestValue implementation, which returns a Object? I thought that doing a test for each type would be redundant. But I see the value of doing a test for each type, we could check if the 'kind' value is being set accordingly. I'll create a helper function and write tests for all types.

[kenzieschmoll]

Consider putting all legacy API tests in a testing group and all async API tests in a testing group

[kenzieschmoll]

can you rename this to sharedPreferencesToolParent or similar so that it is clear that the .parent.parent call is to walk up to the shared_preferences_tool parent? adding a comment could help too

[adsonpleal]

Yes! I was struggling to find a good name for this.

[kenzieschmoll]

does this have any implications when not using the simulated environment?

[adsonpleal]

gonna double check

[adsonpleal]

Just tested it again and we have no implications when running on the actual DevTools.

[kenzieschmoll]

nit: "as a String."

[kenzieschmoll]

nit: remove unnecessary new line between package imports

[kenzieschmoll]

add a comment describing where the event prefix and event kinds are defined