-
Notifications
You must be signed in to change notification settings - Fork 6k
Upgrades to felt (running on multiple modes, multiple backends, single test target option) #22260
Changes from all commits
40848e0
f2213f3
75465a3
eae835f
5889d0c
8117c92
3d7e145
0f13714
dfb5353
78fad8e
9b5f830
2598808
1812790
73fab26
c997661
f373ba6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,15 +28,15 @@ void main() async { | |
| await tester.tap(find.byKey(const Key('input'))); | ||
| // Focus in input, otherwise clipboard will fail with | ||
| // 'document is not focused' platform exception. | ||
| html.document.querySelector('input').focus(); | ||
| html.document.querySelector('input')?.focus(); | ||
| await Clipboard.setData(const ClipboardData(text: 'sample text')); | ||
| }, skip: true); // https://github.com/flutter/flutter/issues/54296 | ||
|
|
||
| testWidgets('Should create and dispose view embedder', | ||
| (WidgetTester tester) async { | ||
| int viewInstanceCount = 0; | ||
|
|
||
| final int currentViewId = platformViewsRegistry.getNextPlatformViewId(); | ||
| platformViewsRegistry.getNextPlatformViewId(); | ||
| // ignore: undefined_prefixed_name | ||
| ui.platformViewRegistry.registerViewFactory('MyView', (int viewId) { | ||
| ++viewInstanceCount; | ||
|
|
@@ -46,14 +46,11 @@ void main() async { | |
| app.main(); | ||
| await tester.pumpAndSettle(); | ||
| final Map<String, dynamic> createArgs = <String, dynamic>{ | ||
| 'id': '567', | ||
| 'id': 567, | ||
| 'viewType': 'MyView', | ||
| }; | ||
| await SystemChannels.platform_views.invokeMethod<void>('create', createArgs); | ||
| final Map<String, dynamic> disposeArgs = <String, dynamic>{ | ||
| 'id': '567', | ||
| }; | ||
| await SystemChannels.platform_views.invokeMethod<void>('dispose', disposeArgs); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm trying to run these tests on different modes. This error really confused me. Unless I change the arguments for However when I read the framework code the arguments are also send as a Do you have any opinion on this type error? (I think there can be some other breakages in the code, if the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting. I don't why it didn't fail before. According to the logic in Is it possible that this test was failing before but the failure didn't register? |
||
| await SystemChannels.platform_views.invokeMethod<void>('dispose', 567); | ||
| expect(viewInstanceCount, 1); | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,9 @@ import 'package:flutter/material.dart'; | |
| import 'package:integration_test/integration_test.dart'; | ||
|
|
||
| void main() { | ||
| final IntegrationTestWidgetsFlutterBinding binding = IntegrationTestWidgetsFlutterBinding.ensureInitialized() as IntegrationTestWidgetsFlutterBinding; | ||
| final IntegrationTestWidgetsFlutterBinding binding = | ||
| IntegrationTestWidgetsFlutterBinding.ensureInitialized() | ||
| as IntegrationTestWidgetsFlutterBinding; | ||
|
|
||
| testWidgets('Focused text field creates a native input element', | ||
| (WidgetTester tester) async { | ||
|
|
@@ -38,7 +40,7 @@ void main() { | |
|
|
||
| // Change the value of the TextFormField. | ||
| final TextFormField textFormField = tester.widget(finder); | ||
| textFormField.controller.text = 'New Value'; | ||
| textFormField.controller?.text = 'New Value'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! Getting an error in this case is better. @mdebbar Yes, it does not give an error, no assignment.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above, if this is not null safe code, you shouldn't need to change this line.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As stated above the reason of the change is the tests are not able to run in debug mode otherwise. |
||
| // DOM element's value also changes. | ||
| expect(input.value, 'New Value'); | ||
|
|
||
|
|
@@ -68,7 +70,7 @@ void main() { | |
|
|
||
| // Change the value of the TextFormField. | ||
| final TextFormField textFormField = tester.widget(finder); | ||
| textFormField.controller.text = 'New Value'; | ||
| textFormField.controller?.text = 'New Value'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As stated above the reason of the change is the tests are not able to run in debug mode otherwise. |
||
| // DOM element's value also changes. | ||
| expect(input.value, 'New Value'); | ||
| }); | ||
|
|
@@ -145,9 +147,9 @@ void main() { | |
| expect(input2.value, 'Text2'); | ||
| }); | ||
|
|
||
| testWidgets('Jump between TextFormFields with tab key after CapsLock is' | ||
| 'activated', | ||
| (WidgetTester tester) async { | ||
| testWidgets( | ||
| 'Jump between TextFormFields with tab key after CapsLock is' | ||
| 'activated', (WidgetTester tester) async { | ||
| app.main(); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
|
|
@@ -163,7 +165,7 @@ void main() { | |
| final List<Node> nodeList = document.getElementsByTagName('input'); | ||
| expect(nodeList.length, equals(1)); | ||
| final InputElement input = | ||
| document.getElementsByTagName('input')[0] as InputElement; | ||
| document.getElementsByTagName('input')[0] as InputElement; | ||
|
|
||
| // Press and release CapsLock. | ||
| dispatchKeyboardEvent(input, 'keydown', <String, dynamic>{ | ||
|
|
@@ -207,7 +209,7 @@ void main() { | |
| // A native input element for the next TextField should be attached to the | ||
| // DOM. | ||
| final InputElement input2 = | ||
| document.getElementsByTagName('input')[0] as InputElement; | ||
| document.getElementsByTagName('input')[0] as InputElement; | ||
| expect(input2.value, 'Text2'); | ||
| }); | ||
|
|
||
|
|
@@ -243,8 +245,8 @@ void main() { | |
| expect(input.hasAttribute('readonly'), isTrue); | ||
|
|
||
| // Make sure the entire text is selected. | ||
| TextRange range = | ||
| TextRange(start: input.selectionStart, end: input.selectionEnd); | ||
| TextRange range = TextRange( | ||
| start: input.selectionStart ?? 0, end: input.selectionEnd ?? 0); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it does not change the result for one this particular method is covering. This is an integration tests, the other conditions related to text selection ranges are covered in the unit tests. The purpose of the test is to see if the text in the selection range is 'Lorem'. Whether
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is |
||
| expect(range.textInside(text), text); | ||
|
|
||
| // Double tap to select the first word. | ||
|
|
@@ -257,7 +259,8 @@ void main() { | |
| await gesture.up(); | ||
| await gesture.down(firstWordOffset); | ||
| await gesture.up(); | ||
| range = TextRange(start: input.selectionStart, end: input.selectionEnd); | ||
| range = TextRange( | ||
| start: input.selectionStart ?? 0, end: input.selectionEnd ?? 0); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. |
||
| expect(range.textInside(text), 'Lorem'); | ||
|
|
||
| // Double tap to select the last word. | ||
|
|
@@ -270,7 +273,8 @@ void main() { | |
| await gesture.up(); | ||
| await gesture.down(lastWordOffset); | ||
| await gesture.up(); | ||
| range = TextRange(start: input.selectionStart, end: input.selectionEnd); | ||
| range = TextRange( | ||
| start: input.selectionStart ?? 0, end: input.selectionEnd ?? 0); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. |
||
| expect(range.textInside(text), 'amet'); | ||
| }); | ||
| } | ||
|
|
||
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 this be
!instead of?? I imagine the test should fail if it cannot find the input.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.
Makes sense, thanks!
Uh oh!
There was an error while loading. Please reload this page.
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.
Unfortunately I'm getting the following error:
Looks like I will wait for these changes until the packet is moved to nnbd.
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.
In that case you don't need to change this line at all, because
querySelector('input').focus()in old Dart is the same asquerySelector('input')!.focus()in new Dart.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.
Then I'm getting this exception in debug mode:
The same tests passes in profile mode, so we didn't need this change before.
Let me know if you have any suggestions.
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.
@jonahwilliams For these tests we are seeing conflicting results for different modes:
Any ideas?
@dnfield have you seen it this issue for other e2e tests?