[web] adding a test for e2e web testing. #2554

nturgut · 2020-02-25T01:49:45Z

Description

(1) Adding a timeout to e2e so any test that doesn't use the MethodChannel won't timeout.
(2) Adding a test for web use case, altering the existing tests.
(3) Altering the previous code not to use dart:io on web
(4) Changing the documentation for future users.
(5) Adding the test to CI

Related Issues

Fixes: flutter/flutter#50212
Fixes: flutter/flutter#50654

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.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

collinjackson · 2020-02-25T03:38:11Z

.cirrus.yml

+        - cd web_installers/packages/web_drivers/
+        - pub get
+        - dart lib/web_driver_installer.dart &
+        - sleep 20


Can we make this more deterministic? Maybe look at the output of web_driver_installer and use that to decide when to call flutter drive?

https://superuser.com/questions/375223/watch-the-output-of-a-command-until-a-particular-string-is-observed-and-then-e

Hmmm... Why is the previous command run in the background, though?

Wouldn't you be able to remove the sleep if the & at the end of dart lib/web_driver_installer.dart was removed?

No it should always run on the background. The code is installing (downloading over the web and extracting. It takes some time) the chrome driver. It also starts running it on port 4444

Ahhh... maybe splitting the code in "the part that downloads and installs" (being synchronous) and the part that "starts the driver on port 444 in the background" could be split in two separate commands, that way you wouldn't need the sleep (maybe)

What I can do is, I can only use web_driver installers for installing. and for launching I can use
chromedriver --port=4444 &

This can remove the need for sleep since it is starts instantly.

If that works, I think it's more deterministic than having the sleep there (imagine that installing takes 21 seconds for whatever reason)

@ditman I sent you a PR to fix the webdriver command (flutter/web_installers#4)

@nturgut just approved it!

packages/e2e/lib/e2e.dart

collinjackson · 2020-02-25T03:46:09Z

packages/e2e/example/test_driver/failure.dart

+    app.main();
+
+    // Trigger a frame.
+    await tester.pumpAndSettle();


Would it be possible to await expectLater below instead of calling this?

I worry about littering tests with tester.pumpAndSettle(), it seems kind of mysterious whether it's needed or not. In any case, I hope that WidgetTester synchronization will help.

collinjackson · 2020-02-25T03:49:29Z

packages/e2e/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.2.4+4
+
+* Add timeout to invokeMethod call, for tests do not use MethodChannel.


Suggested change

* Add timeout to invokeMethod call, for tests do not use MethodChannel.

* Fixed a hang that occurred on platforms that don't have a `MethodChannel` listener registered.

nturgut · 2020-02-25T21:53:47Z

The chrome version flutter/plugins CI is using is 78. It was not supported in web_installers. I'll update web_installers and continue on the PR.

Thanks for the comments.

…unning . use background_task to run it. use ENV for directory change

…of driver

…failing

collinjackson

I think this approach will solve the build issue: collinjackson@7fa057d

collinjackson · 2020-02-27T23:20:18Z

packages/e2e/example/test_driver/example_e2e_web_test.dart

@@ -0,0 +1,15 @@
+@TestOn('browser')


This isn't a test file, you can remove this

collinjackson · 2020-02-27T23:20:24Z

packages/e2e/example/test_driver/example_e2e_web_test.dart

+import 'dart:io';
+
+import 'package:flutter_driver/flutter_driver.dart';
+import 'package:test/test.dart';


This isn't a test file, you can remove this

ditman · 2020-02-28T01:01:50Z

I think this approach will solve the build issue: collinjackson/plugins@7fa057d

That's a very nice trick @collinjackson

nturgut · 2020-02-28T02:05:50Z

I think this approach will solve the build issue: collinjackson@7fa057d

it didn't work unfortunately: https://cirrus-ci.com/task/6188264717549568 It still wants to compile each file and in case of example_e2e_web_test it is not able to.

collinjackson · 2020-02-28T02:17:52Z

It still wants to compile each file and in case of example_e2e_web_test it is not able to.

Actually it looks like the conditional import is working and the tests are running successfully on mobile. Your commit differs from my suggestion in that you left example_e2e_web_test.dart in place and that's where things are not working. You should delete it.

See https://github.com/flutter/plugin_tools/blob/master/lib/src/drive_examples_command.dart#L52

nturgut · 2020-02-28T02:49:04Z

example_e2e_web_test.dart

May be I am missing something, but as far as I know if the file <test_name>_test.dart, does not exist we won't be able to use the following smoke test command:

flutter drive -v --target=test_driver/example_e2e_web.dart -d web-server --release --browser-name=chrome

My suggestions are:
(1) web smoke test will be in another directory
(2) I can change the plugin_tools to run only examples does not contain web

Let me know if these makes sense.

This reverts commit 7cf24d6.

collinjackson · 2020-02-28T14:38:53Z

example_e2e_web_test.dart

May be I am missing something, but as far as I know if the file <test_name>_test.dart, does not exist we won't be able to use the following smoke test command:

flutter drive -v --target=test_driver/example_e2e_web.dart -d web-server --release --browser-name=chrome

My suggestions are:
(1) web smoke test will be in another directory
(2) I can change the plugin_tools to run only examples does not contain web

Let me know if these makes sense.

My current proposal is to remove example_e2e_web_test.dart and you'd have two options for running the web integration test:

flutter drive -v --target=test_driver/example_e2e.dart -d web-server --release --browser-name=chrome would run the main tests entry point and import the web tests. it won't require much of a change to our existing CI, and you're following existing conventions for how to find the driver boilerplate file from the e2e test.
flutter drive -v --target=test_driver/example_e2e_web.dart -d web-server --release --browser-name=chrome --driver=example_e2e_test.dart would run the web test directly. If you go this route you wouldn't need the conditional import, but you'd be introducing more conventions around filenames that might be brittle.

As for moving the web tests to their own folder -- I am not sure about using folder names to determine the platform of tests. It's one thing for us to do this for our own CI, but plugin developers are going to mimic whatever we're doing here, and an approach based on directory structure baked into all our tools seems less flexible to me than a code solution.

nturgut · 2020-02-28T19:05:40Z

example_e2e_web_test.dart

May be I am missing something, but as far as I know if the file <test_name>_test.dart, does not exist we won't be able to use the following smoke test command:
flutter drive -v --target=test_driver/example_e2e_web.dart -d web-server --release --browser-name=chrome
My suggestions are:
(1) web smoke test will be in another directory
(2) I can change the plugin_tools to run only examples does not contain web
Let me know if these makes sense.

My current proposal is to remove example_e2e_web_test.dart and you'd have two options for running the web integration test:

flutter drive -v --target=test_driver/example_e2e.dart -d web-server --release --browser-name=chrome would run the main tests entry point and import the web tests. it won't require much of a change to our existing CI, and you're following existing conventions for how to find the driver boilerplate file from the e2e test.

flutter drive -v --target=test_driver/example_e2e_web.dart -d web-server --release --browser-name=chrome --driver=example_e2e_test.dart would run the web test directly. If you go this route you wouldn't need the conditional import, but you'd be introducing more conventions around filenames that might be brittle.

As for moving the web tests to their own folder -- I am not sure about using folder names to determine the platform of tests. It's one thing for us to do this for our own CI, but plugin developers are going to mimic whatever we're doing here, and an approach based on directory structure baked into all our tools seems less flexible to me than a code solution.

The code runs the tests only if there are under example folder, so if I add a new folder such as web_example or web_smoke it should be ok.

The reason I don't want to remove example_e2e_web_test.dart is soon it would be very different than example_e2e_test.dart by content due to screenshots taken by the driver for web. In the original PR we added more functionality to the driver test: flutter/flutter#49833

For now I can go with the options you suggested but for the future we still need to reconsider differences between driver files.

collinjackson · 2020-02-28T20:46:16Z

packages/e2e/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.2.4+4
+
+* Fixed a hang that occurred on platforms that don't have a `MethodChannel` listener registered..


Suggested change

* Fixed a hang that occurred on platforms that don't have a `MethodChannel` listener registered..

* Fixed a hang that occurred on platforms that don't have a `MethodChannel` listener registered.

* adding a test for e2e web testing. * chaning the changelog. updating pubspec.yaml version. fixing analyze * merging the changelog * addressing reviewer comments * fix format. addressing reviewer comments * try to run chromedriver on the backend * chrome driver is was running as the main task, preventing test from running . use background_task to run it. use ENV for directory change * change in scripts. remove list from backfround task. change argument of driver * removed background task. it wasn't using the same path * run web tests only on browser * add test on to the web driver test as well. drive-examples are still failing * fix the imports * trying reviever suggestion for drive_example compile errors * test _ imports * Revert "test _ imports" This reverts commit 7cf24d6. * removing the web_test driver file upon reviewers suggestion * format fix

nturgut requested review from collinjackson and ditman February 25, 2020 01:49

nturgut requested a review from digiter as a code owner February 25, 2020 01:49

googlebot added the cla: yes label Feb 25, 2020

nturgut removed the request for review from digiter February 25, 2020 01:49

nturgut force-pushed the e2e_smoke_test_web branch from de4c57f to 5a85587 Compare February 25, 2020 02:01

collinjackson reviewed Feb 25, 2020

View reviewed changes

nturgut mentioned this pull request Feb 26, 2020

Change the webdriver package to run with install-only flag. flutter/web_installers#4

Merged

nturgut force-pushed the e2e_smoke_test_web branch 2 times, most recently from bfd8fb7 to b8d4e22 Compare February 27, 2020 19:57

Nurhan Turgut added 12 commits February 27, 2020 15:22

adding a test for e2e web testing.

fb8d623

chaning the changelog. updating pubspec.yaml version. fixing analyze

fd85c78

merging the changelog

a6705e7

addressing reviewer comments

07db3eb

fix format. addressing reviewer comments

749b623

try to run chromedriver on the backend

f553931

chrome driver is was running as the main task, preventing test from r…

abd5cb8

…unning . use background_task to run it. use ENV for directory change

change in scripts. remove list from backfround task. change argument …

6e29ec0

…of driver

removed background task. it wasn't using the same path

e090939

run web tests only on browser

c5ca0c2

add test on to the web driver test as well. drive-examples are still …

bbe1ee9

…failing

fix the imports

a5de564

collinjackson reviewed Feb 27, 2020

View reviewed changes

trying reviever suggestion for drive_example compile errors

494c8d4

nturgut force-pushed the e2e_smoke_test_web branch from ee48546 to 494c8d4 Compare February 28, 2020 01:52

test _ imports

7cf24d6

Revert "test _ imports"

d9cd39e

This reverts commit 7cf24d6.

nturgut added 2 commits February 28, 2020 11:09

removing the web_test driver file upon reviewers suggestion

86abbd6

format fix

d3ad5d2

nturgut requested a review from collinjackson February 28, 2020 19:45

collinjackson approved these changes Feb 28, 2020

View reviewed changes

nturgut merged commit 2349099 into flutter:master Feb 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[web] adding a test for e2e web testing. #2554

[web] adding a test for e2e web testing. #2554

nturgut commented Feb 25, 2020 •

edited

Loading

collinjackson Feb 25, 2020

ditman Feb 25, 2020 •

edited

Loading

nturgut Feb 25, 2020 •

edited

Loading

ditman Feb 25, 2020

nturgut Feb 25, 2020

ditman Feb 25, 2020

nturgut Feb 26, 2020 •

edited

Loading

ditman Feb 26, 2020

nturgut Feb 26, 2020

collinjackson Feb 25, 2020

nturgut Feb 26, 2020

collinjackson Feb 25, 2020

nturgut Feb 26, 2020

nturgut commented Feb 25, 2020 •

edited

Loading

collinjackson left a comment

collinjackson Feb 27, 2020

collinjackson Feb 27, 2020

ditman commented Feb 28, 2020

nturgut commented Feb 28, 2020

collinjackson commented Feb 28, 2020

nturgut commented Feb 28, 2020

collinjackson commented Feb 28, 2020 •

edited

Loading

nturgut commented Feb 28, 2020

collinjackson Feb 28, 2020

		@@ -1,3 +1,7 @@
		## 0.2.4+4

		* Add timeout to invokeMethod call, for tests do not use MethodChannel.

	* Add timeout to invokeMethod call, for tests do not use MethodChannel.
	* Fixed a hang that occurred on platforms that don't have a `MethodChannel` listener registered.

[web] adding a test for e2e web testing. #2554

[web] adding a test for e2e web testing. #2554

Conversation

nturgut commented Feb 25, 2020 • edited Loading

Description

Related Issues

Checklist

Breaking Change

Choose a reason for hiding this comment

ditman Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

nturgut Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nturgut Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nturgut commented Feb 25, 2020 • edited Loading

collinjackson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ditman commented Feb 28, 2020

nturgut commented Feb 28, 2020

collinjackson commented Feb 28, 2020

nturgut commented Feb 28, 2020

collinjackson commented Feb 28, 2020 • edited Loading

nturgut commented Feb 28, 2020

Choose a reason for hiding this comment

nturgut commented Feb 25, 2020 •

edited

Loading

ditman Feb 25, 2020 •

edited

Loading

nturgut Feb 25, 2020 •

edited

Loading

nturgut Feb 26, 2020 •

edited

Loading

nturgut commented Feb 25, 2020 •

edited

Loading

collinjackson commented Feb 28, 2020 •

edited

Loading