Skip to content

Fix stack trace parsing on non-debug builds; add e2e tests#50652

Merged
yjbanov merged 6 commits intoflutter:masterfrom
yjbanov:stack-frame-regex
Feb 14, 2020
Merged

Fix stack trace parsing on non-debug builds; add e2e tests#50652
yjbanov merged 6 commits intoflutter:masterfrom
yjbanov:stack-frame-regex

Conversation

@yjbanov
Copy link
Copy Markdown
Contributor

@yjbanov yjbanov commented Feb 12, 2020

Description

Fix stack trace parsing on non-debug builds; add e2e tests.

Related Issues

Fixes #48909

Tests

I added the following tests:

dev/integration_tests/web/lib/stack_trace.dart, an integration test that checks our ability to parse stack traces in debug, profile, and release modes.

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 read the Tree Hygiene wiki page, which explains my responsibilities.
  • 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

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.
    • I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
    • I got input from the developer relations team, specifically from: Replace with the names of who gave advice
    • I wrote a migration guide: Replace with a link to your migration guide

@fluttergithubbot fluttergithubbot added framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Feb 12, 2020
@fluttergithubbot
Copy link
Copy Markdown
Contributor

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.

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

@yjbanov
Copy link
Copy Markdown
Contributor Author

yjbanov commented Feb 12, 2020

So everyone doesn't have to look at everything we can split the review into the following areas:

  • @dnfield could check the stack trace parsing logic and the corresponding test.
  • @jonahwilliams could check flutter_tools changes.
  • @nturgut could check the browser launching changes (the code is now shared between benchmarks and the test).

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.

Make this RegExp static

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@yjbanov
Copy link
Copy Markdown
Contributor Author

yjbanov commented Feb 12, 2020

BTW, I'm still debugging these changes on Cirrus, but everything is passing on my local workstation so it should be good enough for review.

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.

Specify it is only for chrome devtools

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is it always going to be Chrome-only? How about "on browsers that support it"? This option is hidden anyway. The expectation is that the user knows what they are doing.

Copy link
Copy Markdown
Contributor

@jonahwilliams jonahwilliams Feb 12, 2020

Choose a reason for hiding this comment

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

Probably, I would still specify. This is documentation for the tools team too, someone who isn't experienced on the web isn't going to known chrome devtools vs other browsers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

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.

Should be some existing tests for chrome, you'll want to verify that we use the right port in the launch command. I recomend something with FakeProcessManager.list([...expected launch command])

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Copy Markdown
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.

tools LGTM

Copy link
Copy Markdown
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

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.

note: since this only uses the version available on the system and on the engine we are running a pinned version, the results might be different.

Can you add a printout saying which version of chrome is being used? (If it is easy to do)

Therefore if something fails unexpectedly we can also compare the versions by looking at the log.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done.

@jonahwilliams
Copy link
Copy Markdown
Contributor

Debug stack trace fix in #50680

@yjbanov yjbanov merged commit b340469 into flutter:master Feb 14, 2020
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Uncaught TypeError: Cannot read property '_match' of undefined

7 participants