Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[flutter_plugin_tool] Refactor createFakePlugin #4064

Merged

Conversation

stuartmorgan
Copy link
Contributor

createFakePlugin is used extensively in tests, but has become more and more complex over time. This substantially refactors it and related test utilities:

  • Extracts a createFakePackage for the cases where we want non-plugin packages (allowing the removal of isFlutter from createFakePlugin).
  • Makes most things created automatically instead of being opt-in, so that tests are operating on things that look more like actual packages/plugins unless they specifically override that behavior:
    • Version is added by default.
    • An example/ directory is added by default.
    • A CHANGELOG is added (it can be deleted manually if we ever need to test for that).
  • Combines the two different ways of defining the example format.
  • Replaces the list of platform boolean with a map of platform name to details. This allows more control over how things are set in the pubspec (currently, federated vs inline), rather than requiring rewriting the pubspec after creation.
  • Uses the platform constants in more places instead of raw strings, and renames them to reflect the more general use.
  • Eliminates almost all calls to createFakePubspec, as it is almost never needed any more.

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. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed 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.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

@stuartmorgan
Copy link
Contributor Author

A lot of these changes are pretty mechanical. utils.dart is where most of the interesting changes are.

isIosPlugin: true);
final Directory pluginDirectory =
createFakePlugin('plugin', packagesDir, extraFiles: <List<String>>[
<String>['example', 'test_driver', 'integration_test.dart'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it would be a bit more concise if this was just a list of strings that are paths, then we replaced the '/' with platform specific characters. No need to address it here, just calling it out.

<String>[
        'example/test_driver/integration_test.dart',
        'example/integration_test/bar_test.dart',
        ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that may actually already work by accident, but I'll look at formalizing it. I agree the current system is needlessly messy.

''';
if (isFlutter) {
if (isPlugin) {
yaml += '''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Instead of manipulating strings, we could create the datastructure then use a yaml library to generate the yaml. That would make this a bit more maintainable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've thought about that as well. Initial investigation turned up mostly libraries that can read YAML, unfortunately, rather than write it, but I'd like to look into it more at some point.

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, it was a lot to look at. I just had a few nits.

@stuartmorgan stuartmorgan added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Jun 17, 2021
@fluttergithubbot fluttergithubbot merged commit a5d642d into flutter:master Jun 17, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2021
fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021
@stuartmorgan stuartmorgan deleted the tool-fake-plugin-refactor branch April 19, 2022 17:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
3 participants