Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Take screenshot when drive fails #78822

Merged
merged 1 commit into from
Mar 23, 2021
Merged

Conversation

jmagman
Copy link
Member

@jmagman jmagman commented Mar 22, 2021

Add --screenshot option to flutter drive to take a screenshot when the tests fail. This should make it easier to debug timeout failures like #78805. Since it doesn't rely on VM's screenshot this should also help us detect that the app launched but is just sitting on a white screen because the engine failed to initialize.

Adopt in devicelab tests to point to the directory that gets uploaded via LUCI recipe.

I purposely regressed #75665 locally, and lo:

[ios_platform_view_tests] [STDOUT] stdout: [   +7 ms] 01:01 +0 -2: Some tests failed.
[ios_platform_view_tests] [STDOUT] stderr: [   +1 ms] Unhandled exception:
[ios_platform_view_tests] [STDOUT] stderr: [        ] Dummy exception to set exit code.
[ios_platform_view_tests] [STDOUT] stdout: [  +59 ms] executing: /Users/magder/Projects/flutter/bin/cache/artifacts/libimobiledevice/idevicescreenshot /var/folders/bx/pyplw7v92lj58gm3_lztz5sc00mfq2/T/flutter_test_logs.qTqzs5/drive_01.png --udid d83d5bc53967baa0ee18626ba87b6254b2ab5418
[ios_platform_view_tests] [STDOUT] stdout: [ +601 ms] Screenshot saved to /var/folders/bx/pyplw7v92lj58gm3_lztz5sc00mfq2/T/flutter_test_logs.qTqzs5/drive_01.png
[ios_platform_view_tests] [STDOUT] stdout: [        ] Screenshot written to /var/folders/bx/pyplw7v92lj58gm3_lztz5sc00mfq2/T/flutter_test_logs.qTqzs5/drive_01.png

drive_01

I wouldn't have had to SSH into the host to figure out the device was sideways.

@jmagman jmagman added team Infra upgrades, team productivity, code health, technical debt. See also team: labels. tool Affects the "flutter" command-line tool. See also t: labels. labels Mar 22, 2021
@jmagman jmagman self-assigned this Mar 22, 2021
@flutter-dashboard flutter-dashboard bot added the a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) label Mar 22, 2021
@google-cla google-cla bot added the cla: yes label Mar 22, 2021
expect(logger.statusText, contains('Screenshot written to drive_screenshots/drive_01.png'));
});

testWithoutContext('drive --screenshot errors but does not fail if screenshot fails', () async {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried for awhile to get DriveCommand working with the CommandRunner to actually run the test, but I eventually gave up after I couldn't pass in --no-pub

// `pub` must always be run due to the test script running from source,
// even if an application binary is used.
@override
bool get shouldRunPub => true;

throw UnsupportedError('Attempted to invoke pub during test.');

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I think this is wrong. We should always run pub by default, unless we're asked not to.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Past Jonah says #70136

@jmagman
Copy link
Member Author

jmagman commented Mar 23, 2021

Let's ignore the Cirrus failures for now as long as the LUCI equivalents pass #78678.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I can take a look at the pub issue tomorrow

@jmagman jmagman merged commit 3463946 into flutter:master Mar 23, 2021
@jmagman jmagman deleted the screenshot-drive branch March 23, 2021 02:06
@jmagman
Copy link
Member Author

jmagman commented Mar 23, 2021

(yeessssss.... #66153 (comment))

@@ -292,6 +293,8 @@ TaskFunction createStackSizeTest() {
'--driver', testDriver,
'-d',
deviceId,
'--screenshot',
hostAgent.dumpDirectory.path,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into issue where local environment does not have FLUTTER_LOGS_DIR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a regression from #84008. Fix out at #84820

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) team Infra upgrades, team productivity, code health, technical debt. See also team: labels. tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants