-
Notifications
You must be signed in to change notification settings - Fork 27k
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.
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
Add "excluding" optional parameter to TargetPlatformVariant to communicate cases where test should be ran everywhere but specific platforms #106216
Conversation
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 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>{} |
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.
Comma at the end here.
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)); |
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: Maybe also expect that the current target platform is not in excludePlatforms?
|
||
|
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.
Remove these two extra newlines.
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.
@
packages/flutter/test/material/flexible_space_bar_collapse_mode_test.dart
Show resolved
Hide resolved
@@ -47,7 +47,7 @@ void main() { | |||
|
|||
expect(topBeforeScroll.dy, equals(0.0)); | |||
expect(topAfterScroll.dy, equals(0.0)); | |||
}, variant: TargetPlatformVariant(TargetPlatform.values.where((TargetPlatform value) => value != TargetPlatform.fuchsia).toSet())); |
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.
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.
}, 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 { |
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.
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 { |
…o communicate cases where test should be ran everywhere but specific platforms (flutter/flutter#106216)
… to communicate cases where test should be ran everywhere but specific platforms (flutter/flutter#106216)
… to communicate cases where test should be ran everywhere but specific platforms (flutter/flutter#106216)
…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 <oleina@google.com>
… to communicate cases where test should be ran everywhere but specific platforms (flutter/flutter#106216)
…o communicate cases where test should be ran everywhere but specific platforms (flutter/flutter#106216)
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
///
).