Upgrades to felt (running on multiple modes, multiple backends, single test target option) #22260

nturgut · 2020-11-03T01:33:58Z

Description

Changing felt :

run integration tests on all three build modes for Chrome and profile/release mode for other browsers.
run integration tests on both rendering backends (html and canvaskit)
adding a blocked list for cases where the tests are not passing/suppose to run on this platform.
adding a flag to be able to control the rendering backend via felt
adding a flag to run one single target instead of all integration tests
adding a flag to run on a selected mode (previously only profile mode was available)

TODO: I'll send another PR for updating all the documentations!

Related Issues

Fixes: flutter/flutter#52987
Fixes: flutter/flutter#69734

Tests

Running on LUCI: https://ci.chromium.org/p/flutter/builders/try/Linux%20Web%20Engine/11477?

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
[] All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

flutter-dashboard · 2020-11-03T01:34:03Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

nturgut · 2020-11-03T16:15:30Z

e2etests/web/regular_integration_tests/test_driver/platform_messages_integration.dart

-    final Map<String, dynamic> disposeArgs = <String, dynamic>{
-      'id': '567',
-    };
-    await SystemChannels.platform_views.invokeMethod<void>('dispose', disposeArgs);


@ferhatb @yjbanov @hterkelsen

I'm trying to run these tests on different modes. This error really confused me. Unless I change the arguments for dispose only with an integer, I'm getting the following stack trace:

Failure in method: Should create and dispose view embedder ══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞═════════════════ The following TypeErrorImpl was thrown running a test: Expected a value of type 'int', but got one of type 'LinkedMap<dynamic, dynamic>' When the exception was thrown, this was the stack: dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 216:49 throw_ dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 64:3 castError dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart 442:10 cast dart-sdk/lib/_internal/js_dev_runtime/patch/core_patch.dart 204:12 as lib/ui/ui.dart 139:13 _disposePlatformView lib/ui/ui.dart 108:7 handlePlatformViewCall lib/_engine/engine/platform_dispatcher.dart 380:11 [_sendPlatformMessage] lib/_engine/engine/platform_dispatcher.dart 216:5 sendPlatformMessage lib/ui/src/ui/window.dart 116:24 sendPlatformMessage packages/flutter/src/services/system_channels.dart.js 2184:17 [_sendPlatformMessage] packages/flutter/src/services/system_channels.dart.js 2228:40 send packages/flutter_test/src/_matchers_web.dart.js 3264:40 send packages/flutter/src/services/system_channels.dart.js 938:50 _invokeMethod dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 84:54 runBody dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 123:12 _async packages/flutter/src/services/system_channels.dart.js 936:20 [_invokeMethod] packages/flutter/src/services/system_channels.dart.js 950:33 invokeMethod platform_messages_integration.dart.js 79:63 <fn> dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 45:50 <fn> packages/stack_trace/src/stack_zone_specification.dart.js 163:98 <fn> packages/stack_trace/src/stack_zone_specification.dart.js 231:16 [_run] packages/stack_trace/src/stack_zone_specification.dart.js 163:80 <fn> dart-sdk/lib/async/zone.dart 1198:47 _rootRunUnary dart-sdk/lib/async/zone.dart 1100:19 runUnary dart-sdk/lib/async/future_impl.dart 143:18 handleValue dart-sdk/lib/async/future_impl.dart 696:44 handleValueCallback dart-sdk/lib/async/future_impl.dart 725:32 _propagateToListeners dart-sdk/lib/async/future_impl.dart 529:5 [_completeWithValue] dart-sdk/lib/async/future_impl.dart 567:7 <fn> packages/stack_trace/src/stack_zone_specification.dart.js 231:16 [_run] packages/stack_trace/src/stack_zone_specification.dart.js 154:71 <fn> dart-sdk/lib/async/zone.dart 1190:13 _rootRun dart-sdk/lib/async/zone.dart 1093:19 run dart-sdk/lib/async/zone.dart 997:7 runGuarded dart-sdk/lib/async/zone.dart 1037:23 callback dart-sdk/lib/async/schedule_microtask.dart 41:11 _microtaskLoop dart-sdk/lib/async/schedule_microtask.dart 50:5 _startMicrotaskLoop dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 166:15 <fn> The test description was: Should create and dispose view embedder

However when I read the framework code the arguments are also send as a <String, dynamic> map: https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/services/platform_views.dart#L945

Do you have any opinion on this type error? (I think there can be some other breakages in the code, if the <String, dynamic> is the correct argument set but started causing an error due to a recent change`)

Interesting. I don't why it didn't fail before. According to the logic in _disposePlatformView we are expecting an int, not a map: final int id = methodCall.arguments;

Is it possible that this test was failing before but the failure didn't register?

yjbanov · 2020-11-05T00:54:49Z

e2etests/web/regular_integration_tests/test_driver/platform_messages_integration.dart

    // Focus in input, otherwise clipboard will fail with
    // 'document is not focused' platform exception.
-    html.document.querySelector('input').focus();
+    html.document.querySelector('input')?.focus();


Should this be ! instead of ?? I imagine the test should fail if it cannot find the input.

Makes sense, thanks!

Unfortunately I'm getting the following error:

Target dart2js failed: Exception: test_driver/text_editing_integration.dart:43:29: Error: Null safety features are disabled for this library. textFormField.controller!.text = 'New Value';

Looks like I will wait for these changes until the packet is moved to nnbd.

In that case you don't need to change this line at all, because querySelector('input').focus() in old Dart is the same as querySelector('input')!.focus() in new Dart.

Then I'm getting this exception in debug mode:

org-dartlang-app:/platform_messages_integration.dart:31:42: Error: Method 'focus' cannot be called on 'Element?' because it is potentially null. - 'Element' is from 'dart:html'. Try calling using ?. instead. html.document.querySelector('input').focus();

The same tests passes in profile mode, so we didn't need this change before.

Let me know if you have any suggestions.

@jonahwilliams For these tests we are seeing conflicting results for different modes:

debug mode has a nnbd error:

org-dartlang-app:/platform_messages_integration.dart:31:42: Error: Method 'focus' cannot be called on 'Element?' because it is potentially null. - 'Element' is from 'dart:html'. Try calling using ?. instead. html.document.querySelector('input').focus();

profile/release modes do not recognize nnbd:

Target dart2js failed: Exception: test_driver/text_editing_integration.dart:43:29: Error: Null safety features are disabled for this library.

Any ideas?

@dnfield have you seen it this issue for other e2e tests?

yjbanov · 2020-11-05T00:58:43Z

e2etests/web/regular_integration_tests/test_driver/platform_messages_integration.dart

-    final Map<String, dynamic> disposeArgs = <String, dynamic>{
-      'id': '567',
-    };
-    await SystemChannels.platform_views.invokeMethod<void>('dispose', disposeArgs);


Interesting. I don't why it didn't fail before. According to the logic in _disposePlatformView we are expecting an int, not a map: final int id = methodCall.arguments;

Is it possible that this test was failing before but the failure didn't register?

yjbanov · 2020-11-05T00:59:50Z

e2etests/web/regular_integration_tests/test_driver/text_editing_integration.dart

    // Change the value of the TextFormField.
    final TextFormField textFormField = tester.widget(finder);
-    textFormField.controller.text = 'New Value';
+    textFormField.controller?.text = 'New Value';


Should this use ! instead? Is it a valid situation that textFormField.controller is null?

What happens if textFormField.controller is null? Does it just skip the assignment?

Thanks! Getting an error in this case is better.

@mdebbar Yes, it does not give an error, no assignment.

Same as above.

Same as above, if this is not null safe code, you shouldn't need to change this line.

As stated above the reason of the change is the tests are not able to run in debug mode otherwise.

yjbanov · 2020-11-05T00:59:55Z

e2etests/web/regular_integration_tests/test_driver/text_editing_integration.dart

    // Change the value of the TextFormField.
    final TextFormField textFormField = tester.widget(finder);
-    textFormField.controller.text = 'New Value';
+    textFormField.controller?.text = 'New Value';


same as above.

Same as above :)

As stated above the reason of the change is the tests are not able to run in debug mode otherwise.

yjbanov · 2020-11-05T01:01:16Z

e2etests/web/regular_integration_tests/test_driver/text_editing_integration.dart

-    TextRange range =
-        TextRange(start: input.selectionStart, end: input.selectionEnd);
+    TextRange range = TextRange(
+        start: input.selectionStart ?? 0, end: input.selectionEnd ?? 0);


?? is essentially an if/else, which means two paths are possible, one when input.selectionStart is null and another when it isn't. However, a test is only executed once, which means only one situation should be possible, and the other situation is likely a failure of some sort.

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 input.end being null or 0 will give the same outcome, since the selection range won't be 'Lorem' in either case, this method should fail.

Is input being null semantically equivalent to input.end being 0? Or would it indicate a bug in the framework? If it's the latter, then failing due to "Lorem" string not being selected would be misleading because the actual bug is that the framework sent null, which was not expected. If they both mean the same thing, then I guess it's OK to fallback to zeros. It still feels strange to me that we don't know what the framework will send.

lib/web_ui/dev/integration_tests_manager.dart

yjbanov · 2020-11-05T01:30:26Z

lib/web_ui/dev/integration_tests_manager.dart

+/// If a test is not suppose to run for one of the modes also add that test
+/// to the corresponding list.
+// TODO(nurhan): Remove the failing test after fixing.
+const Map<String, List<String>> blockedTestsListsMapForModes =


It's still weird that one needs to recompile felt in order to change the list of tests they want to run. I'd prefer a YAML file that lives closer to the test code itself.

Since it's been a while we added blocked tests logic, it seems like the request is outside of this PR. I create another issue and address that change there.

yjbanov · 2020-11-05T01:31:51Z

lib/web_ui/dev/integration_tests_manager.dart

  ],
 };
+
+/// Tests blocked for one of the build modes.


It is more common to call such tests "skipped" rather than "blocked".

It is outside of the scope of this work. It has been a while since we added 'blocked' test concept. Blocked word is introduced as a replacement to 'blacklist' a while ago.

lib/web_ui/dev/test_runner.dart

nturgut · 2020-11-09T19:03:58Z

@yjbanov for question on #22260 (comment) I think you refer to the case that if the test fails but the exit code is still "0". I checked logs of few weeks back, test failures, it is not on the logs.

yjbanov

LGTM. Remaining comments are non-blocking.

nturgut · 2020-11-11T23:37:45Z

Retrying mac web engine build: https://ci.chromium.org/p/flutter/builders/try/Mac%20Web%20Engine/11454

nturgut · 2020-11-12T01:15:54Z

@yjbanov I'm merging this one, so that we can at least catch the errors better.
This PR is missing auto backend and release mode. I'll add them after I shard the recipe.

…s, single test target option) (flutter/engine#22260)

…e test target option) (flutter#22260) * testing running the tests on all build modes * don't run debug mode on other browsers * fix platform message test failures * some cleanup. change dispose platform channel message * adding flags to control the integration tests better with felt * running tests by target name, selecting web rendering backend * fix conditions * carrying some conditions to helper methods. Adding comments * create a blocked list for failing canvaskit test * parse parameters before all integration tests * Give better warning to developers for tests that are blocked for CI * address some reviwer comments (more remains) * remove named parameters * also run with auto mode * add verbose option * reduce the number of tests running. skip url_test for now

google-cla bot added the cla: yes label Nov 3, 2020

nturgut commented Nov 3, 2020

View reviewed changes

nturgut force-pushed the intg_tests_debug_mode branch from d1226ec to 462e7c2 Compare November 3, 2020 23:53

nturgut requested review from ferhatb, harryterkelsen and yjbanov November 4, 2020 20:44

nturgut changed the title ~~testing running the tests on all build modes~~ Upgrades to felt (running on multiple modes, multiple backends, single test target option) Nov 4, 2020

yjbanov reviewed Nov 5, 2020

View reviewed changes

chinmaygarde added the platform-web Code specifically for the web engine label Nov 5, 2020

yjbanov approved these changes Nov 10, 2020

View reviewed changes

nturgut added 15 commits November 10, 2020 14:45

testing running the tests on all build modes

40848e0

don't run debug mode on other browsers

f2213f3

fix platform message test failures

75465a3

some cleanup. change dispose platform channel message

eae835f

adding flags to control the integration tests better with felt

5889d0c

running tests by target name, selecting web rendering backend

8117c92

fix conditions

3d7e145

carrying some conditions to helper methods. Adding comments

0f13714

create a blocked list for failing canvaskit test

dfb5353

parse parameters before all integration tests

78fad8e

Give better warning to developers for tests that are blocked for CI

9b5f830

address some reviwer comments (more remains)

2598808

remove named parameters

1812790

also run with auto mode

73fab26

add verbose option

c997661

nturgut force-pushed the intg_tests_debug_mode branch from af3ab2b to c997661 Compare November 11, 2020 01:19

reduce the number of tests running. skip url_test for now

f373ba6

nturgut merged commit 4d44b7e into flutter:master Nov 12, 2020

engine-flutter-autoroll mentioned this pull request Nov 12, 2020

Roll Engine from d97a81c88983 to 4d44b7e7d927 (10 revisions) flutter/flutter#70348

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 12, 2020

4d44b7e Upgrades to felt (running on multiple modes, multiple backend…

e2b8b56

…s, single test target option) (flutter/engine#22260)

engine-flutter-autoroll mentioned this pull request Nov 12, 2020

Roll Engine from d97a81c88983 to cdc44862e72f (11 revisions) flutter/flutter#70353

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 12, 2020

4d44b7e Upgrades to felt (running on multiple modes, multiple backend…

831b283

…s, single test target option) (flutter/engine#22260)

engine-flutter-autoroll mentioned this pull request Nov 12, 2020

Roll Engine from d97a81c88983 to cdc44862e72f (11 revisions) flutter/flutter#70361

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 12, 2020

4d44b7e Upgrades to felt (running on multiple modes, multiple backend…

9e60d03

…s, single test target option) (flutter/engine#22260)

engine-flutter-autoroll mentioned this pull request Nov 13, 2020

Roll Engine from c5c7e83b9e53 to d07368169e3b (35 revisions) flutter/flutter#70417

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 13, 2020

4d44b7e Upgrades to felt (running on multiple modes, multiple backend…

2333924

…s, single test target option) (flutter/engine#22260)

engine-flutter-autoroll mentioned this pull request Nov 13, 2020

Roll Engine from c5c7e83b9e53 to 1ff237fdca45 (38 revisions) flutter/flutter#70425

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 13, 2020

4d44b7e Upgrades to felt (running on multiple modes, multiple backend…

21c4781

…s, single test target option) (flutter/engine#22260)

engine-flutter-autoroll mentioned this pull request Nov 13, 2020

Roll Engine from c5c7e83b9e53 to dddb532b5c7b (40 revisions) flutter/flutter#70429

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 13, 2020

4d44b7e Upgrades to felt (running on multiple modes, multiple backend…

486bee6

…s, single test target option) (flutter/engine#22260)

gaaclarke mentioned this pull request Nov 13, 2020

Dart version check tripping up on copyright notices with local engine flutter/flutter#70498

Closed

Upgrades to felt (running on multiple modes, multiple backends, single test target option) #22260

Upgrades to felt (running on multiple modes, multiple backends, single test target option) #22260

Uh oh!

Conversation

nturgut commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

Uh oh!

flutter-dashboard bot commented Nov 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nturgut Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nturgut commented Nov 9, 2020

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

nturgut commented Nov 11, 2020

Uh oh!

nturgut commented Nov 12, 2020

nturgut commented Nov 3, 2020 •

edited

Loading

nturgut Nov 9, 2020 •

edited

Loading