-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[webview_flutter] Fix Flaky Resize Test #4460
[webview_flutter] Fix Flaky Resize Test #4460
Conversation
| ), | ||
| ); | ||
| )); | ||
| await tester.pump(Duration(seconds: 3)); |
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 is required to wait for the initial resize of the WebView. The line await pageFinishedCompleter.future; can finish before this.
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.
Please capture this in a code comment, as it's non-obvious.
stuartmorgan-g
left a comment
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 with nits.
(Related though: do we need three copies of this test? It's not clear to me where exactly the functionality that it's intended to test lives; are there important parts that live in both native an app-facing-package-dart?)
| ), | ||
| ); | ||
| )); | ||
| await tester.pump(Duration(seconds: 3)); |
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.
Please capture this in a code comment, as it's non-obvious.
| ), | ||
| ); | ||
| await tester.tap(find.byKey(const ValueKey('resizeButton'))); | ||
| await tester.pump(Duration(seconds: 3)); |
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.
How were the 3-second values chosen? It seems like a very arbitrary number.
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.
It is mostly arbitrarily chosen :/. I just chose a safe value to wait for. I considered just waiting for the first call to resize, but I wasn't 100% sure it gets called every time on every platform.
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.
Wait, so it this always waiting 3 seconds? I'm not all that familiar with widget tests; I assumed this was a timeout. If it's a static 3 second wait, that's still going to be a source of flake.
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.
That's a good point. I changed the tests to wait for resize completers.
stuartmorgan-g
left a comment
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
Split of #4455
After reading through https://flutter.dev/docs/cookbook/testing/widget/introduction. It seems that this test isn't using a Widget test in the expected way.After more testing, the current solution does work in updating the widget, but still has a false positive for the 2nd reason below.1. It's attempting to change the size of aWebViewby pumping a newWidget.2. I was able to make this test fail by adding a
await Future.delayed(Duration(seconds: 2));before thebooleancheck. This shows that resize is called regardless of changing the width and height.This PR follows the Flutter cookbook example of a Widget test and creates a widget that changes the WebView on a button press. It also waits for the initial resize of the WebView from the initial load.
Pre-launch Checklist
dart format.)[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.