Skip to content

[flutter_tools] fix null check in crash reporter#80382

Merged
jonahwilliams merged 5 commits into
flutter:masterfrom
jonahwilliams:fix_craash_reporter
Apr 13, 2021
Merged

[flutter_tools] fix null check in crash reporter#80382
jonahwilliams merged 5 commits into
flutter:masterfrom
jonahwilliams:fix_craash_reporter

Conversation

@jonahwilliams

@jonahwilliams jonahwilliams commented Apr 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

The crash reporter is currently broken, it will crash while trying to report a crash. We need a test for this. Perhaps some way to force the flutter_tool to produce a crash report?

Fixes #80383

@flutter-dashboard

flutter-dashboard Bot commented Apr 13, 2021

Copy link
Copy Markdown

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

@flutter-dashboard flutter-dashboard Bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 13, 2021
@jonahwilliams jonahwilliams requested a review from jmagman April 13, 2021 17:48
@google-cla google-cla Bot added the cla: yes label Apr 13, 2021
@jonahwilliams

jonahwilliams commented Apr 13, 2021

Copy link
Copy Markdown
Contributor Author

This broke in 8ddc27e which is unfortunately in the latest RC so we'll need to cherry pick.

jmagman
jmagman approved these changes Apr 13, 2021
View reviewed changes

@jmagman jmagman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@christopherfujino

christopherfujino commented Apr 13, 2021

Copy link
Copy Markdown
Contributor

for testing, can we add a method that always throws with @visibleForTesting?

@jonahwilliams

jonahwilliams commented Apr 13, 2021

Copy link
Copy Markdown
Contributor Author

I added a flag to update-packages, since that is really only a command for contributors and is already hidden. This allows us to simulate a crash with as few differences as possible between the test environment and IRL.

christopherfujino
christopherfujino approved these changes Apr 13, 2021
View reviewed changes

@christopherfujino christopherfujino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@jonahwilliams jonahwilliams mentioned this pull request Apr 13, 2021
Closed
@jmagman

jmagman commented Apr 13, 2021

Copy link
Copy Markdown
Member

Actual output is missing the Oops

  Expected: contains 'Oops; flutter has exited unexpectedly: "Bad state: test crash please ignore.".\n'
              'A crash report has been written to'
    Actual: '\n'
              'Bad state: test crash please ignore.\n'

@jonahwilliams

jonahwilliams commented Apr 13, 2021

Copy link
Copy Markdown
Contributor Author

Ahh, the oops is stdout

@jonahwilliams

jonahwilliams commented Apr 13, 2021

Copy link
Copy Markdown
Contributor Author

err, maybe its a terminal versus no terminal thing

@jonahwilliams

jonahwilliams commented Apr 13, 2021

Copy link
Copy Markdown
Contributor Author

g3 failure seems unrelated so I'll land on green.

@jonahwilliams jonahwilliams merged commit b7214a9 into flutter:master Apr 13, 2021
@jonahwilliams jonahwilliams deleted the fix_craash_reporter branch April 13, 2021 21:04
christopherfujino pushed a commit to chris-forks/flutter that referenced this pull request Apr 14, 2021
@christopherfujino
[flutter_tools] fix null check in crash reporter (flutter#80382)
ecd0beb
christopherfujino added a commit that referenced this pull request Apr 15, 2021
@christopherfujino
[flutter_releases] Flutter Dev 2.2.0-10.1.pre Framework Cherrypicks (#…
0941968 
…80459)

* [flutter_tools] fix null check in crash reporter (#80382)
* roll engine cherrypicks
* Roll engine with dart_style cp

Co-authored-by: Jonah Williams <jonahwilliams@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmagman jmagman jmagman approved these changes
+1 more reviewer
@christopherfujino christopherfujino christopherfujino 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.

Crash reporter may be unable to report crashes in current RC branch

3 participants

@jonahwilliams @christopherfujino @jmagman