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

[flutter_plugin_tools] Make unit tests pass on Windows #4149

Merged
merged 19 commits into from
Jul 9, 2021

Conversation

stuartmorgan
Copy link
Contributor

@stuartmorgan stuartmorgan commented Jul 9, 2021

The purpose of this PR is to make running all unit tests on Windows pass (vs failing a large portion of the tests as currently happens). This does not mean that the commands actually work when run on Windows, or that Windows support is tested, only that it's possible to actually run the tests themselves. This is prep for actually supporting parts of the tool on Windows in future PRs.

Major changes:

  • Make the tests significantly more hermetic:
    • Make almost all tools take a Platform constructor argument that can be used to inject a mock platform to control what OS the command acts like it is running on under test.
    • Add a path Context object to the base command, whose style matches the Platform, and use that almost everywhere instead of the top-level path functions.
    • In cases where Posix behavior is always required (such as parsing git output), explicitly use the posix context object for path functions.
  • Start laying the groundwork for actual Windows support:
    • Replace all uses of flutter as a command with a getter that returns flutter or flutter.bat as appropriate.
    • For user messages that include relative paths, use a helper that always uses Posix-style relative paths for consistent output.

Part of flutter/flutter#86113

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 stuartmorgan requested a review from ditman July 9, 2021 14:29
@google-cla google-cla bot added the cla: yes label Jul 9, 2021
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

Jeez, what a change! Thanks for taking the time to normalize this. Even though this is going to be mostly internal development, is there any way that we can test automatically that people are using "p.posix", "path" and "Platform" consistently through their fixes? Or add some literature somewhere?

This is a pattern that seems "easy" to abandon unless it's enforced somehow.

@stuartmorgan
Copy link
Contributor Author

stuartmorgan commented Jul 9, 2021

is there any way that we can test automatically that people are using "p.posix", "path" and "Platform" consistently through their fixes?

The plan is to start bringing commands online for Windows one at a time, and as I do I'll be adding tests that run in Windows mode. So the enforcement will be in not breaking those tests when making changes.

@stuartmorgan stuartmorgan merged commit 1199f13 into flutter:master Jul 9, 2021
@stuartmorgan stuartmorgan deleted the tools-windows-unittests branch July 9, 2021 23:38
@ditman
Copy link
Member

ditman commented Jul 10, 2021

So the enforcement will be in not breaking those tests when making changes.

Ah, of course, makes sense!

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 10, 2021
amantoux pushed a commit to amantoux/plugins that referenced this pull request Jul 10, 2021
The purpose of this PR is to make running all unit tests on Windows pass (vs failing a large portion of the tests as currently happens). This does not mean that the commands actually work when run on Windows, or that Windows support is tested, only that it's possible to actually run the tests themselves. This is prep for actually supporting parts of the tool on Windows in future PRs.

Major changes:
- Make the tests significantly more hermetic:
  - Make almost all tools take a `Platform` constructor argument that can be used to inject a mock platform to control what OS the command acts like it is running on under test.
  - Add a path `Context` object to the base command, whose style matches the `Platform`, and use that almost everywhere instead of the top-level `path` functions.
  - In cases where Posix behavior is always required (such as parsing `git` output), explicitly use the `posix` context object for `path` functions.
- Start laying the groundwork for actual Windows support:
  - Replace all uses of `flutter` as a command with a getter that returns `flutter` or `flutter.bat` as appropriate.
  - For user messages that include relative paths, use a helper that always uses Posix-style relative paths for consistent output.

This bumps the version since quite a few changes have built up, and having a cut point before starting to make more changes to the commands to support Windows seems like a good idea.

Part of flutter/flutter#86113
Ralph-Li added a commit to Insight-Timer/plugins that referenced this pull request Jul 15, 2021
* upstream_master: (40 commits)
  [image_picker] Image picker fix camera device (flutter#3898)
  [flutter_plugin_tools] Improve license-check output (flutter#4154)
  [webview_flutter] Fix broken keyboard issue link (flutter#3266)
  [flutter_plugin_tools] Support format on Windows (flutter#4150)
  [flutter_plugin_tools] Make unit tests pass on Windows (flutter#4149)
  [image_picker_for_web] Migrate image_picker to package:cross_file (flutter#4083)
  [various] Prepare plugin repo for binding API improvements (flutter#4148)
  [quick_actions] Add const constructor (flutter#4131)
  [in_app_purchase] Add iOS currency symbol to ProductDetails (flutter#4144)
  [in_app_purchase] Added priceCurrencySymbol to SkuDetailsWrapper (flutter#4114)
  [image_picker_platform_interface] Add methods that return package:cross_file (flutter#4072)
  [flutter_plugin_tools] Improve and test 'format' (flutter#4145)
  [flutter_plugin_tools] Only check target packages in analyze (flutter#4146)
  [in_app_purchase] Fix crash when retrieveReceiptWithError gives an error. (flutter#4138)
  [video_player] Pause video when it completes (flutter#3727)
  [in_app_purchase] Add currencySymbol to ProductDetails (flutter#4115)
  [in_app_purchase] Add documentation for price change confirmations (flutter#4092)
  [camera] android-rework part 8: Supporting modules for final implementation (flutter#4054)
  [plugin_platform_interface] Fix README broken link (flutter#4143)
  [various] Prepare plugin repo for binding API improvements (flutter#4137)
  ...
fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021
The purpose of this PR is to make running all unit tests on Windows pass (vs failing a large portion of the tests as currently happens). This does not mean that the commands actually work when run on Windows, or that Windows support is tested, only that it's possible to actually run the tests themselves. This is prep for actually supporting parts of the tool on Windows in future PRs.

Major changes:
- Make the tests significantly more hermetic:
  - Make almost all tools take a `Platform` constructor argument that can be used to inject a mock platform to control what OS the command acts like it is running on under test.
  - Add a path `Context` object to the base command, whose style matches the `Platform`, and use that almost everywhere instead of the top-level `path` functions.
  - In cases where Posix behavior is always required (such as parsing `git` output), explicitly use the `posix` context object for `path` functions.
- Start laying the groundwork for actual Windows support:
  - Replace all uses of `flutter` as a command with a getter that returns `flutter` or `flutter.bat` as appropriate.
  - For user messages that include relative paths, use a helper that always uses Posix-style relative paths for consistent output.

This bumps the version since quite a few changes have built up, and having a cut point before starting to make more changes to the commands to support Windows seems like a good idea.

Part of flutter/flutter#86113
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants