Skip to content

Take screenshot when drive fails #78822

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.

Sign up for GitHub

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

Jump to bottom
Merged
jmagman merged 1 commit into from
Mar 23, 2021
Merged

Take screenshot when drive fails #78822

jmagman merged 1 commit into from
Mar 23, 2021

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