-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Fix webview_flutter Android integration tests and add Espresso #4147
Conversation
…t=/Users/bmparr/Development/plugins/../test_driver/webview_flutter_test.dart
@@ -9,6 +9,7 @@ | |||
<application | |||
android:icon="@mipmap/ic_launcher" | |||
android:label="webview_flutter_example" | |||
android:usesCleartextTraffic="true" |
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.
For the espresso package
@@ -532,6 +535,7 @@ void main() { | |||
expect(fullScreen, _webviewBool(false)); | |||
}); | |||
|
|||
// allowsInlineMediaPlayback is a noop on Android, so it is skipped. |
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.
This test fails when running on Android, but it doesn't seem like there is anything we can do since a flag like this isn't supported on Android. At least according to the comment.
@@ -1272,18 +1276,19 @@ void main() { | |||
), | |||
); | |||
final WebViewController controller = await controllerCompleter.future; | |||
await controller.evaluateJavascript('window.open("about:blank", "_blank")'); | |||
await controller | |||
.evaluateJavascript('window.open("https://flutter.dev/", "_blank")'); |
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.
This test would stall on Android. For some reason, Android has different behavior when a window.open
is called with a url with https://
vs not. It loads normally when using a URL with https://
, but appends the url to the current url when it doesnt. (e.g. https://flutter.dev/
turns into https://flutter.dev/about:blank
)
await pageLoaded.future; | ||
final String? currentUrl = await controller.currentUrl(); | ||
expect(currentUrl, 'about:blank'); | ||
expect(currentUrl, 'https://flutter.dev/'); | ||
}); | ||
|
||
testWidgets( | ||
'can open new window and go back', |
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.
Fixed the weird bug where pageLoaded.complete()
was called in the callback multiple times. From the Dart side, this caused a silent error and prevented the rest of the method calls from going through.
...e/android/app/src/androidTest/java/io/flutter/plugins/webviewflutterexample/WebViewTest.java
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,21 @@ | |||
<manifest xmlns:android="http://schemas.android.com/apk/res/android" |
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.
This file is needed to use WebViewTestActivity
.
@@ -56,7 +56,8 @@ flutter { | |||
|
|||
dependencies { | |||
testImplementation 'junit:junit:4.12' | |||
testImplementation "com.google.truth:truth:1.0" |
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.
this doesn't seem used, is it?
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.
LGTM!
@stuartmorgan @blasten I think this PR is ready to go, but the tests seem to be pretty flaky. Is it alright to submit this if the integration tests require 2-3 runs to pass? Should I investigate more about why they are flaky or could I just skip the flaky ones for now? |
Let's skip the flaky tests, with a bug filed to follow up (please CC me, and mark it |
@stuartmorgan @blasten I skipped the flaky tests and added a link to flutter/flutter#86757. PTAL and I'll merge |
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.
Wow, that's a lot of flakes :(
But LGTM
@@ -0,0 +1,17 @@ | |||
<manifest xmlns:android="http://schemas.android.com/apk/res/android" | |||
package="io.flutter.plugins.webviewflutterexample"> | |||
<!-- Flutter needs it to communicate with the running application |
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.
"needs internet permission to"
import io.flutter.embedding.android.FlutterActivity; | ||
import io.flutter.embedding.engine.FlutterEngine; | ||
|
||
// Extends FlutterActivity to make the FlutterEngine accessible for testing. |
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.
what about: Makes the FlutterEngine accessible for testing.
Integration tests weren't running since they were in the
androidTestDebug
folder and not aandroidTest
folder. This changes the folder name and fix the tests that weren't running.Part of: flutter/flutter#83358
Pre-launch Checklist
dart format
.)///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.