Skip to content

Fix Dart plugin registrant interaction with 'flutter test' #90288

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.

Sign up for GitHub

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

Jump to bottom
Merged
stuartmorgan-g merged 7 commits into from
Oct 1, 2021
Merged

Fix Dart plugin registrant interaction with 'flutter test' #90288

stuartmorgan-g merged 7 commits into from
Oct 1, 2021

Conversation

@stuartmorgan-g
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g commented Sep 17, 2021 •
edited
Loading

Building an application for a desktop platform that transitively included any Dart-based plugins (such as path_provider) broke flutter test, because its compilation was overriding the provided main (in this case, the test main) with generated_main.dart if it was present. This PR:

  • Changes the flutter test compilation path to update generated_main.dart, so that the tests will work, and will include any registered Dart plugins.
  • Makes using generated_main.dart during recompile opt-in, to try to reduce the chance of a similar bug happening with other codepaths in the future.

Fixes #88794

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 Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • 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.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Sep 17, 2021
@google-cla google-cla bot added the cla: yes label Sep 17, 2021
@stuartmorgan-g
Copy link
Contributor Author

stuartmorgan-g commented Sep 17, 2021

/cc @blasten It still needs unit tests, and I haven't run all the unit tests yet to see if I need to adjust other test setup, but I've verified that this fixes the e2e test (as well as testing various orderings of build/run/test/drive manually). Feel free to take an early look if you like.

@stuartmorgan-g stuartmorgan-g marked this pull request as draft September 17, 2021 20:43
blasten
blasten reviewed Sep 17, 2021
View reviewed changes
Copy link

@blasten blasten left a comment

Choose a reason for hiding this comment

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

got it. Changes LGTM

@blasten blasten mentioned this pull request Sep 17, 2021
Closed
@stuartmorgan-g stuartmorgan-g marked this pull request as ready for review September 20, 2021 17:40
@stuartmorgan-g
Copy link
Contributor Author

stuartmorgan-g commented Sep 20, 2021

@blasten I added a unit test, and the other tests seems happy, so this is ready for review.

blasten
blasten reviewed Sep 24, 2021
View reviewed changes
packages/flutter_tools/test/general.shard/test/test_compiler_test.dart
expect(generatedMain, exists);
expect(
generatedMain.readAsStringSync(),
contains("import 'test/foo.dart' as entrypoint;")
Copy link

@blasten blasten Sep 24, 2021

Choose a reason for hiding this comment

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

what if someone changes the generated code to // import 'test/foo.dart' as entrypoint;?
could we ensure the entire output?

blasten
blasten approved these changes Sep 24, 2021
View reviewed changes
Copy link

@blasten blasten left a comment

Choose a reason for hiding this comment

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

LGTM

@fertrig
Copy link

fertrig commented Sep 28, 2021

Will this PR also fix flutter run?

We have an issue where our tool runs flutter build bundle, which creates the generated_main.dart file, which imports a main file generated by the tool. Then, when the user runs flutter run with some other main target, the old generated_main.file is still used.

@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Sep 28, 2021

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@stuartmorgan-g
Copy link
Contributor Author

stuartmorgan-g commented Sep 29, 2021

Will this PR also fix flutter run?

I doubt it; I didn't intentionally change anything for the run path.

We have an issue where our tool runs flutter build bundle, which creates the generated_main.dart file, which imports a main file generated by the tool. Then, when the user runs flutter run with some other main target, the old generated_main.file is still used.

Please file an issue with detailed repro steps and @-mention me; the test version is the only issue I was aware of.

@fertrig fertrig mentioned this pull request Sep 29, 2021
Closed
@stuartmorgan-g
Copy link
Contributor Author

stuartmorgan-g commented Oct 1, 2021

Google testing isn't updating for some reason, but I've checked that the associated CL did pass its tests, so I'm going to land.

@stuartmorgan-g stuartmorgan-g merged commit 97fb8c0 into flutter:master Oct 1, 2021
@stuartmorgan-g stuartmorgan-g deleted the dart-plugin-class-test-fix branch October 1, 2021 03:25
This was referenced Oct 1, 2021
Closed
Merged
@stuartmorgan-g stuartmorgan-g mentioned this pull request Oct 12, 2021
Closed
2 tasks
@stuartmorgan-g stuartmorgan-g mentioned this pull request Oct 26, 2021
Closed
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Oct 29, 2021
@stuartmorgan-g @clocksmith
Fix Dart plugin registrant interaction with 'flutter test' (flutter#9…
5530976 
…0288)

Building an application for a desktop platform that transitively included any Dart-based plugins (such as path_provider) broke `flutter test`, because its compilation was overriding the provided main (in this case, the test main) with `generated_main.dart` if it was present. This PR:
- Changes the `flutter test` compilation path to update `generated_main.dart`, so that the tests will work, and will include any registered Dart plugins.
- Makes using `generated_main.dart` during recompile opt-in, to try to reduce the chance of a similar bug happening with other codepaths in the future.

Fixes flutter#88794
@stuartmorgan-g stuartmorgan-g mentioned this pull request Nov 2, 2021
Closed
@alexbatalov alexbatalov mentioned this pull request Dec 19, 2021
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@blasten blasten blasten approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Mac_staging plugin_test_ios keeps failing in staging

5 participants

@stuartmorgan-g @fertrig @fluttergithubbot @blasten @stuartmorgan