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

Fix remaining holes in stack trace demangling #60478

Merged
merged 11 commits into from Jul 6, 2020

Conversation

pingbird
Copy link
Member

Description

Follow-up of #59900

Instead of forcing every implementer of FlutterError.onError (both internally and externally) to manually demangle stack traces, we cover the small number of places in the framework that parse them.

  1. Added a more helpful error message to StackFrame parsing when it encounters package:stack_trace async gap delimiters, this prevents people from assuming the vm's own stack trace format changed, as we all initially assumed.
  2. Added FlutterError.demangleStackTrace to allow testing environments to provide their own demangle callbacks.
  3. Added demangleStackTrace to every place FlutterError.defaultStackFilter or StackFrame.fromStackString could be passed an external stack trace.
  4. Implemented demangleStackTrace in the flutter_test binding.
  5. Added tests for all of the above.

Related Issues

Fixes any remaining instances of #59893

Breaking Change

None of these changes should be breaking, and all local tests are passing.

@fluttergithubbot fluttergithubbot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Jun 28, 2020
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

Only real concern is removing the test - it's not entirely clear to me why we should allow that test to fail now.

@pingbird
Copy link
Member Author

pingbird commented Jul 1, 2020

I actually renamed the file then boiled it down to what it is now, which is basically testing if FlutterErrorDetails.toDiagnosticsNode fails.

Looking back at it, the added redundancy of a slightly more practical use case is probably a good thing so I'll just re-introduce the original test.

@pingbird
Copy link
Member Author

pingbird commented Jul 1, 2020

Weird, your async_gap_test.dart doesn't fail if I comment out the assignment of demangleStackTrace, where was it supposed to fail before?

@dnfield
Copy link
Contributor

dnfield commented Jul 1, 2020

I think as long as the test isn't failing it's fine, but we should keep the test.

If something's broken, the test should end up failing on your new assert that we don't have a line like ====== asynchronous gap etc.

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM

@dnfield dnfield merged commit d986fdc into flutter:master Jul 6, 2020
dnfield added a commit that referenced this pull request Jul 6, 2020
dnfield added a commit that referenced this pull request Jul 6, 2020
jonahwilliams added a commit that referenced this pull request Jul 6, 2020
dnfield added a commit that referenced this pull request Jul 7, 2020
dnfield added a commit that referenced this pull request Jul 7, 2020
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
* Implement stack trace demangling in the framework.
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants