[web] Adding capability to e2e to take screenshot for web tests. #2904
Conversation
packages/e2e/lib/e2e.dart
Outdated
| await _webDriverCommand(WebDriverAction.screenshot(screenshot_name)); | ||
| } | ||
|
|
||
| Future<void> _webDriverCommand(WebDriverAction command) async { |
There was a problem hiding this comment.
Shouldn't this go in a web-specific implementation file, and we'd have another implementation file for IO, so we just call the method (or instantiate the class) and the VM version just throws for now?
There was a problem hiding this comment.
I carried the webdriver specific logic to another file. Please have another look.
packages/e2e/lib/e2e.dart
Outdated
| Map<String, String> params) async { | ||
| final String command = params['command']; | ||
| // Test status is added as an exta message. | ||
| final String extraMessage = params['message'] ?? ''; |
There was a problem hiding this comment.
Do we have this documented anywhere?
Would we be better off implementing some serialized type for this that could more clearly be used on both sides of the message?
There was a problem hiding this comment.
We can add a string to the requestData call for managing generic communication. Documentation is here: https://github.com/flutter/flutter/blob/master/packages/flutter_driver/lib/src/driver/driver.dart#L494
Are you recommending to use another command other than RequestData. I think that would make the handshake more complicated.
packages/e2e/lib/e2e.dart
Outdated
| // Test status is added as an exta message. | ||
| final String extraMessage = params['message'] ?? ''; | ||
| Map<String, String> response; | ||
| switch (command) { |
There was a problem hiding this comment.
IOW, can we avoid having this switch statement that will continue to grow, with nested ifs underneath that will get harder to reason about?
There was a problem hiding this comment.
I refactored the methods and separated the part for request_data into 2 methods. I hope it looks more readable now. PHAL.
packages/e2e/lib/e2e.dart
Outdated
| } else { | ||
| // If Test status is `wait_on_webdriver_command` send the first | ||
| // command in the `commandPipe` to the tests. | ||
| if (extraMessage == '${TestStatus.wait_on_webdriver_command}') { |
There was a problem hiding this comment.
Why would the driving script do this? Shouldn't the test itself be awaiting whatever it needs (i.e. taking the screenshot), and the driving script just waits for the test reporting to finish?
Another way of putting this concern: how does the caller know which web driver command(s) it's waiting on, that there won't be another one it's actually interested in coming right after the one it just waited on?
There was a problem hiding this comment.
Why would the driving script do this? Shouldn't the test itself be awaiting whatever it needs (i.e. taking the screenshot), and the driving script just waits for the test reporting to finish?
Only driver script can take screenshots. Almost all browsers implements the webdriver API which allows us to take screenshot. We can also extend this to do other import things such as changing screensize, contorrling cookies: https://www.w3.org/TR/webdriver1/. Since only driver test can perform these operations, we developed this handshake mechanism between the integration_test and the driver test.
In summary: We are sending the request from the integration_test (ex: WebDriverActionTypes.screenshot), waiting on the result from the driver. If there are multiple test methods which are asking for a WebDriverAction, they wait on the mutex, driver handles them one by one.
Another way of putting this concern: how does the caller know which web driver command(s) it's waiting on, that there won't be another one it's actually interested in coming right after the one it just waited on?
That is why I used a synchronization method (such as mutex) to send these requests to driver one at a time. I tested with many scenarios:
- requests in within different methods.
- few request in a few methods.
- Failing vs passing tests in different methods.
This handshake approach worked well.
nturgut
force-pushed
the
web_screenshots
branch
2 times, most recently
from
0f41cff to
4562479
Compare
packages/integration_test/example/test_driver/example_integration_extended.dart
Show resolved
Hide resolved
nturgut
force-pushed
the
web_screenshots
branch
from
95476a5 to
d860b3c
Compare
| // Trigger a frame. | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| // TODO: Add screenshot capability for mobile platforms. |
There was a problem hiding this comment.
nit: link this to the screenshot bug.
There was a problem hiding this comment.
is there an issue for it for mobile? or should I use the existing issue?
There was a problem hiding this comment.
Just use the existing one.
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
Please remove the .png files, and consider updating the .gitignore for that.
done. thanks for noticing :) |
…ager->callback manager
…n object instead of strings for status messages
…les in integration_test package is common
nturgut
force-pushed
the
web_screenshots
branch
from
4ea736c to
0b3456d
Compare
|
I'm investigating the current breakages of the CI, and they don't seem related to any changes in the flutter/plugins repo (but a change in the engine), so if you need to merge this on red, go ahead! |
thanks a lot for investigating :) |
…tter#2904) * screenshot taking works * squash commits. addressing reviewer comments. making drivercommandmanager->callback manager * addressing reviewer comment. mostly name changes * major rename on all files webdriveraction->webdrivercommand * remove files. use implements * remove timeout. add onScreenshot callback * remove remaning timeouts. add an error message for screenshots. use an object instead of strings for status messages * created a new issue * remove example screenshot saving since it's failing on android. examples in integration_test package is common * changing the version and the change log
…tter#2904) * screenshot taking works * squash commits. addressing reviewer comments. making drivercommandmanager->callback manager * addressing reviewer comment. mostly name changes * major rename on all files webdriveraction->webdrivercommand * remove files. use implements * remove timeout. add onScreenshot callback * remove remaning timeouts. add an error message for screenshots. use an object instead of strings for status messages * created a new issue * remove example screenshot saving since it's failing on android. examples in integration_test package is common * changing the version and the change log
7 participants
Description
Adding capability to e2e to take screenshot for web tests. This infrastructure can also be used for triggering other WebDriver actions such as resize, back, forward. (https://www.w3.org/TR/webdriver/#endpoints)
Manually tested on Chrome, Firefox, Safari and Ios-Safari.
e2e_driver_extended.dartis only to be used as an example. We will use Flutter Gold to do the matching goldens for Flutter for Web integration tests.Related Issues
flutter/flutter#51890
flutter/flutter#62706
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.///).flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?