Skip to content

Catch exceptions thrown by runChecked* when possible #36109

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
blasten merged 5 commits into from
Jul 18, 2019
Merged

Catch exceptions thrown by runChecked* when possible #36109

blasten merged 5 commits into from
Jul 18, 2019

Conversation

@blasten
Copy link

@blasten blasten commented Jul 12, 2019

Description

Catch the exceptions when possible.

Related Issues

#35929

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Jul 12, 2019

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

Reviewers: Read the Tree Hygine page and make sure this patch meets those guidelines before LGTMing.

/cc @dnfield

@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 12, 2019
@blasten blasten force-pushed the run_checked_exception branch from 9b2983b to c1f06a9 Compare July 12, 2019 23:50
@blasten
Copy link
Author

blasten commented Jul 12, 2019

This PR isn't changing behavior.

@codecov
Copy link

codecov bot commented Jul 13, 2019 •
edited
Loading

Codecov Report

Merging #36109 into master will decrease coverage by 0.22%.
The diff coverage is 61.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #36109      +/-   ##
==========================================
- Coverage   54.95%   54.72%   -0.23%     
==========================================
  Files         187      187              
  Lines       17248    17260      +12     
==========================================
- Hits         9478     9445      -33     
- Misses       7770     7815      +45
Flag Coverage Δ
#flutter_tool 54.72% <61.76%> (-0.23%) ⬇️
Impacted Files Coverage Δ
...ckages/flutter_tools/lib/src/commands/upgrade.dart 32.85% <0%> (-0.48%) ⬇️
...ckages/flutter_tools/lib/src/ios/code_signing.dart 98.48% <100%> (+1.6%) ⬆️
...ckages/flutter_tools/lib/src/commands/version.dart 93.33% <100%> (+0.31%) ⬆️
.../flutter_tools/lib/src/android/android_device.dart 33.5% <45.45%> (+2.12%) ⬆️
...ges/flutter_tools/lib/src/application_package.dart 66.29% <75%> (-0.25%) ⬇️
packages/flutter_tools/lib/src/ios/xcodeproj.dart 85.49% <75%> (+2.41%) ⬆️
...ter_tools/lib/src/build_system/targets/assets.dart 5.26% <0%> (-94.74%) ⬇️
...lutter_tools/lib/src/android/android_workflow.dart 45.09% <0%> (-19.61%) ⬇️
...ges/flutter_tools/lib/src/build_system/source.dart 77.14% <0%> (-8.58%) ⬇️
packages/flutter_tools/lib/src/version.dart 95.56% <0%> (-1.98%) ⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa65ddf...0136472. Read the comment docs.

jonahwilliams
jonahwilliams suggested changes Jul 13, 2019
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.

We should still have tests to confirm that we're catch the exceptions we think we are, and responding appropriately

zanderso
zanderso reviewed Jul 15, 2019
View reviewed changes
packages/flutter_tools/lib/src/android/android_device.dart Outdated
await runAdbCheckedAsync(<String>[
'shell', 'echo', '-n', _getSourceSha1(app), '>', _getDeviceSha1Path(app),
]);
} catch (error) {
Copy link
Member

@zanderso zanderso Jul 15, 2019

Choose a reason for hiding this comment

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

Catch ProcessException only here and below.

packages/flutter_tools/lib/src/android/android_device.dart
output = runAdbCheckedSync(<String>[
'shell', '-x', 'logcat', '-v', 'time', '-t', '1',
]);
} catch (error) {
Copy link
Member

@zanderso zanderso Jul 15, 2019

Choose a reason for hiding this comment

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

This should also catch ProcessException only, but that likely requires modifying _runWithLoggingSync in process.dart to throw a ProcessException instead of a String.

Copy link
Author

@blasten blasten Jul 16, 2019

Choose a reason for hiding this comment

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

I filed #36242. What about we change exception type in a separate PR?

Copy link
Member

@zanderso zanderso Jul 16, 2019

Choose a reason for hiding this comment

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

Sure, if you think that will make that change easier.

@blasten blasten force-pushed the run_checked_exception branch from f86f5ab to db9923f Compare July 16, 2019 06:44
@blasten blasten requested a review from zanderso July 16, 2019 06:51
zanderso
zanderso approved these changes Jul 16, 2019
View reviewed changes
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

lgtm on green

@blasten blasten force-pushed the run_checked_exception branch from db9923f to 12e56e3 Compare July 16, 2019 15:32
jonahwilliams
jonahwilliams approved these changes Jul 16, 2019
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!

@blasten blasten merged commit 5a34e79 into flutter:master Jul 18, 2019
@blasten blasten deleted the run_checked_exception branch July 18, 2019 17:45
johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019
@johnsonmh
Catch exceptions thrown by runChecked* when possible (flutter#36109)
014490b
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@zanderso zanderso zanderso approved these changes

+1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

5 participants

@blasten @fluttergithubbot @zanderso @jonahwilliams @googlebot