-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[flutter_plugin_tools] Overhaul drive-examples #4099
[flutter_plugin_tools] Overhaul drive-examples #4099
Conversation
…pported correctly in practice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff was a bit hard to parse, but it looks like all the old tests are still intact, just refactored.
printError('No Android devices available'); | ||
throw ToolExit(_exitNoAvailableDevice); | ||
} | ||
androidDevice = devices.first; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we warn if there are more than one device?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about adding more control over devices via flags, but I figured that absent anyone actually wanting that functionality I'd just do the simplest thing for now, and we can expand later (e.g., adding device choice flags to the command itself) if needed.
return errors; | ||
} | ||
|
||
Future<List<String>> _getDevicesForPlatform(String platform) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Splitting this into parsing the results of devices
and calling devices
would make it more testable. (Although it looks like you can use MockProcess to test it fine.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now relying on the indirect testing is fine; if we end up needing this in more places and pull this out as a utility method we can split it apart and do direct unit testing.
Landing on "red" since |
Significantly restructures drive-examples: - Migrates it to the new package-looping base command - Enforces that only one platform is passed, since in practice multiple platforms never actually worked. (The logic is structured so that it will be easy to enable multi-platform if `flutter drive` gains multi-platform support.) - Fixes the issue where `--ios` and `--android` were semi-broken, by doing explicit device targeting for them rather than relying on the default device being the right kind - Extracts much of the logic to helpers so it's easier to understand the flow - Removes support for a legacy integration test file structure that is no longer used - Adds more test coverage; previously no failure cases were actually tested. Fixes flutter/flutter#85147 Part of flutter/flutter#83413
Significantly restructures drive-examples: - Migrates it to the new package-looping base command - Enforces that only one platform is passed, since in practice multiple platforms never actually worked. (The logic is structured so that it will be easy to enable multi-platform if `flutter drive` gains multi-platform support.) - Fixes the issue where `--ios` and `--android` were semi-broken, by doing explicit device targeting for them rather than relying on the default device being the right kind - Extracts much of the logic to helpers so it's easier to understand the flow - Removes support for a legacy integration test file structure that is no longer used - Adds more test coverage; previously no failure cases were actually tested. Fixes flutter/flutter#85147 Part of flutter/flutter#83413
Significantly restructures drive-examples:
flutter drive
gains multi-platform support.)--ios
and--android
were semi-broken, by doing explicit device targeting for them rather than relying on the default device being the right kindFixes flutter/flutter#85147
Part of flutter/flutter#83413
Pre-launch Checklist
dart format
.)[shared_preferences]
///
).