Add "excluding" optional parameter to TargetPlatformVariant to communicate cases where test should be ran everywhere but specific platforms#106216
Merged
antholeole merged 6 commits intoflutter:masterfrom Jun 22, 2022
Conversation
flutter-dashboard
bot
added
a: tests
antholeole
requested review from
gspencergoog and
justinmc
and removed request for
gspencergoog
justinmc
reviewed
Jun 17, 2022
Contributor
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
I really like this and have found myself wishing it existed before. I think the possibility of Flutter adding a new platform in the future is a strong argument in favor of adding this.
This LGTM but I'm going to hold off on approving to let others chime in that might have more thoughts about expanding this API or not.
| /// the [TargetPlatform] enum. If [excluding] is provided, will test all platforms | ||
| /// except those in [excluding]. | ||
| TargetPlatformVariant.all({ | ||
| Set<TargetPlatform> excluding = const <TargetPlatform>{} |
| const Set<TargetPlatform> excludePlatforms = <TargetPlatform>{ TargetPlatform.android, TargetPlatform.linux }; | ||
| testWidgets('TargetPlatformVariant.all with excluding runs an all variants except those provided in excluding', (WidgetTester tester) async { | ||
| if (debugDefaultTargetPlatformOverride == null) { | ||
| expect(numberOfVariationsRun, equals(TargetPlatform.values.length - excludePlatforms.length)); |
Contributor
There was a problem hiding this comment.
Nit: Maybe also expect that the current target platform is not in excludePlatforms?
Comment on lines
+741
to
+742
|
|
||
|
|
Contributor
There was a problem hiding this comment.
Remove these two extra newlines.
Carlos123z
reviewed
Jun 18, 2022
packages/flutter/test/material/flexible_space_bar_collapse_mode_test.dart
Show resolved
Hide resolved
|
|
||
| expect(topBeforeScroll.dy, equals(0.0)); | ||
| expect(topAfterScroll.dy, equals(0.0)); | ||
| }, variant: TargetPlatformVariant(TargetPlatform.values.where((TargetPlatform value) => value != TargetPlatform.fuchsia).toSet())); |
gspencergoog
approved these changes
Jun 21, 2022
| }, variant: TargetPlatformVariant.all()); | ||
|
|
||
| const Set<TargetPlatform> excludePlatforms = <TargetPlatform>{ TargetPlatform.android, TargetPlatform.linux }; | ||
| testWidgets('TargetPlatformVariant.all with excluding runs an all variants except those provided in excluding', (WidgetTester tester) async { |
Contributor
There was a problem hiding this comment.
Suggested change
| testWidgets('TargetPlatformVariant.all with excluding runs an all variants except those provided in excluding', (WidgetTester tester) async { | |
| testWidgets('TargetPlatformVariant.all, excluding runs an all variants except those provided in excluding', (WidgetTester tester) async { |
8 tasks
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/plugins
that referenced
this pull request
Jun 23, 2022
…o communicate cases where test should be ran everywhere but specific platforms (flutter/flutter#106216)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jun 23, 2022
…o communicate cases where test should be ran everywhere but specific platforms (flutter/flutter#106216)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jun 23, 2022
…o communicate cases where test should be ran everywhere but specific platforms (flutter/flutter#106216)
camsim99
pushed a commit
to camsim99/flutter
that referenced
this pull request
Aug 10, 2022
…icate cases where test should be ran everywhere but specific platforms (flutter#106216) added "excluding" optional parameter to targetPlatforms.all Co-authored-by: Anthony Oleinik <[email protected]>
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 30, 2022
…o communicate cases where test should be ran everywhere but specific platforms (flutter/flutter#106216)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/plugins
that referenced
this pull request
Aug 30, 2022
…o communicate cases where test should be ran everywhere but specific platforms (flutter/flutter#106216)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added an optional "excluding" flag for when the user wants to run a test on all platforms but some specific platforms. I also changed some occurances where the writer of the test could have used "excluding" but didn't have it available.
I think in a lot of the cases, the writer of the test listed all the platforms besides the apple platforms - what they probably meant was that they wanted the test run on all the platforms besides the apple ones, but resorted to just manually writing them all out. In the case that the flutter team decides to add more platforms (are there even any more!), this case would be considered broken since a test that just hard codes "all platforms but apple" would have not listed the new platforms (I actually think there are some tests that are being skipped on windows that shouldn't be?). By using "excluding" the author can communicate that what they mean is they want to run that test on all platforms besides the ones listed.
It seems like in a ton of places in the code, we're hard coding in all the other platforms besides one or two - I'd like to replace them with excluding, but their intention is now lost with time: perhaps they actually meant they wanted to only run on those 4 platforms, so the substitution cannot be made.
I'd recommend looking at this commit by commit, since the first commit is the change and the test, and the second commit just changes some previous tests where this would apply.
List which issues are fixed by this PR. You must list at least one issue.
No issues 😬 should I make one?
If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].
Pre-launch Checklist
///).