Skip to content

Take screenshot when drive fails#78822

Merged
jmagman merged 1 commit intoflutter:masterfrom
jmagman:screenshot-drive
Mar 23, 2021
Merged

Take screenshot when drive fails#78822
jmagman merged 1 commit intoflutter:masterfrom
jmagman:screenshot-drive

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Mar 22, 2021 •
edited
Loading

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 c: contributor-productivity Team-specific productivity, code health, technical debt. 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
jmagman
jmagman commented Mar 22, 2021
View reviewed changes
packages/flutter_tools/test/commands.shard/hermetic/drive_test.dart Outdated
Copy link
Member Author

@jmagman jmagman Mar 22, 2021

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

flutter/packages/flutter_tools/lib/src/commands/drive.dart

Lines 142 to 145 in ce318b7

// `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;

flutter/packages/flutter_tools/test/src/throwing_pub.dart

Line 37 in 74bd7b6

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

Copy link
Contributor

@jonahwilliams jonahwilliams Mar 23, 2021

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

@jmagman jmagman Mar 23, 2021

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 jmagman requested a review from jonahwilliams March 23, 2021 00:39
@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.

jonahwilliams
jonahwilliams approved these changes Mar 23, 2021
View reviewed changes
Copy link
Contributor

@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))

@jmagman jmagman mentioned this pull request Jun 3, 2021
Closed
chunhtai
chunhtai reviewed Jun 18, 2021
View reviewed changes
dev/devicelab/lib/tasks/perf_tests.dart
'-d',
deviceId,
'--screenshot',
hostAgent.dumpDirectory.path,
Copy link
Contributor

@chunhtai chunhtai Jun 18, 2021

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

@jmagman jmagman Jun 18, 2021

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

@jmagman jmagman mentioned this pull request Jan 19, 2022
Merged
8 tasks
@jmagman jmagman mentioned this pull request Jan 31, 2024
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chunhtai chunhtai chunhtai left review comments

+1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jmagman jmagman

Labels

a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jmagman @jonahwilliams @chunhtai