Skip to content

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

Merged
stuartmorgan-g merged 7 commits intoflutter:masterfrom
stuartmorgan-g:dart-plugin-class-test-fix
Oct 1, 2021
Merged

Fix Dart plugin registrant interaction with 'flutter test'#90288
stuartmorgan-g merged 7 commits intoflutter:masterfrom
stuartmorgan-g:dart-plugin-class-test-fix

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