Run tests in selectable_region_context_menu_test.dart#185380
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the platform-specific testing logic in selectable_region_context_menu_test.dart by replacing a boolean check with TargetPlatformVariant. Feedback indicates that several tests now lack the necessary skip condition for non-web environments, which will lead to failures due to package:web dependencies. It is also suggested to use a final variable for the variant definition instead of a getter to optimize performance by avoiding redundant allocations.
|
|
||
| final int numberOfStyleElements = getNumberOfStyleElements(); | ||
| expect(numberOfStyleElements, 1); | ||
| }, variant: _browserContextMenuEnabledVariants); |
There was a problem hiding this comment.
The removal of the kIsWeb check will cause this test to fail when run on the VM because it depends on package:web (e.g., web.HTMLElement). Please restore the skip condition to ensure it only runs in a browser environment. The // [intended] comment is also standard practice in this repository for intentional skips. Alternatively, if this file is intended to be web-only, you could add @TestOn('browser') at the top of the file.
}, variant: _browserContextMenuEnabledVariants, skip: !kIsWeb); // [intended]There was a problem hiding this comment.
@TestOn('browser') // This file contains web-only library.
library;already exists at the top of file.
|
|
||
| final int numberOfStyleElements = getNumberOfStyleElements(); | ||
| expect(numberOfStyleElements, 1); | ||
| }, variant: _browserContextMenuEnabledVariants); |
There was a problem hiding this comment.
There was a problem hiding this comment.
@TestOn('browser') // This file contains web-only library.
library;already exists at the top of file.
| expect(selectWordEvent, isNotNull); | ||
| expect((selectWordEvent!.globalPosition.dx - 200).abs() < precisionErrorTolerance, isTrue); | ||
| expect((selectWordEvent.globalPosition.dy - 300).abs() < precisionErrorTolerance, isTrue); | ||
| }, variant: _browserContextMenuEnabledVariants); |
There was a problem hiding this comment.
There was a problem hiding this comment.
@TestOn('browser') // This file contains web-only library.
library;already exists at the top of file.
| element.dispatchEvent(event); | ||
| expect(event.defaultPrevented, isTrue); | ||
| } | ||
| }, variant: _browserContextMenuEnabledVariants); |
There was a problem hiding this comment.
There was a problem hiding this comment.
@TestOn('browser') // This file contains web-only library.
library;already exists at the top of file.
| TargetPlatformVariant get _browserContextMenuEnabledVariants => TargetPlatformVariant( | ||
| <TargetPlatform>{...TargetPlatformVariant.desktop().values, TargetPlatform.fuchsia}, | ||
| ); |
There was a problem hiding this comment.
Defining the variant as a final variable is preferred over a getter to avoid redundant allocations of the TargetPlatformVariant and its underlying Set on every test case execution.
final TargetPlatformVariant _browserContextMenuEnabledVariants = TargetPlatformVariant(
<TargetPlatform>{...TargetPlatformVariant.desktop().values, TargetPlatform.fuchsia},
);References
- Suggest simplification and refactoring to enhance maintainability. (link)
|
An existing Git SHA, To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with |
b850ed1 to
5c2d61c
Compare
| TargetPlatform.linux, | ||
| TargetPlatform.macOS, | ||
| TargetPlatform.windows, | ||
| }, |
There was a problem hiding this comment.
I wonder if we should do something like:
const TargetPlatformVariant _browserContextMenuEnabledVariants = TargetPlatformVariant(
TargetPlatform.values
.where((platform) =>
platform != TargetPlatform.android &&
platform != TargetPlatform.iOS
)
.toSet(),
);This would make sure this opt-out continues to work even if we add more TargetPlatforms in the future. I don't feel strongly though, I'd be happy with the current approach too.
There was a problem hiding this comment.
Good point sgtm!
|
autosubmit label was removed for flutter/flutter/185380, because - The status or check suite Windows framework_tests_misc has failed. Please fix the issues identified (or deflake) before re-applying this label. |
Fixes an issue where
selectable_region_context_menu_test.darttests were being skipped.flutter test --platform=chrome /..../selectable_region_context_menu_test.dartalways defaults toTargetPlatform.androidand tests were being explicitly skipped onTargetPlatform.androidsince they do not support the browser context menu at this time. BecauseTargetPlatform.androidis the default then that means every tests in this file was being skipped.This change fixes the issue by leveraging the
variantmember oftestWidgetsinstead of theskipmember that was being used to explicitly skip tests onTargetPlatform.android. Now we run tests onTargetPlatform.androidbut only for the supported variants (linux, mac, windows, fuschia).Pre-launch Checklist
///).