Skip to content

Add Driver command to get diagnostics tree#34440

Merged
goderbauer merged 6 commits intoflutter:masterfrom
goderbauer:diagdriver
Jun 19, 2019
Merged

Add Driver command to get diagnostics tree#34440
goderbauer merged 6 commits intoflutter:masterfrom
goderbauer:diagdriver

Conversation

@goderbauer
Copy link
Member

@goderbauer goderbauer commented Jun 13, 2019

Description

Adds commands to get diagnostics tree for widgets and render objects to Flutter Driver.

Related Issues

Fixes #29375.

Tests

I added the following tests:

  • tests for the new command

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.

@goderbauer
Copy link
Member Author

goderbauer commented Jun 13, 2019

/cc @adazh

@goderbauer goderbauer requested a review from tvolkert June 13, 2019 21:08
@goderbauer goderbauer changed the title Add command to get diagnostics tree to Driver Add Driver command to get diagnostics tree Jun 13, 2019
@goderbauer
Copy link
Member Author

goderbauer commented Jun 13, 2019

/cc @jacob314

adazh
adazh reviewed Jun 13, 2019
View reviewed changes
dnfield
dnfield reviewed Jun 14, 2019
View reviewed changes
packages/flutter_driver/lib/src/extension/extension.dart Outdated
Copy link
Contributor

@dnfield dnfield Jun 14, 2019

Choose a reason for hiding this comment

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

Should this be DiagnosticsType.element then? And the associated method called getElementDiagnostics?

Copy link
Member Author

@goderbauer goderbauer Jun 14, 2019

Choose a reason for hiding this comment

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

This is consistent with the terminology we use in the Widget Inspector for these two trees.

dnfield
dnfield approved these changes Jun 14, 2019
View reviewed changes
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

@Piinks Piinks added a: tests "flutter test", flutter_test, or one of our tests customer: espresso framework flutter/packages/flutter repository. See also f: labels. labels Jun 14, 2019
tvolkert
tvolkert reviewed Jun 14, 2019
View reviewed changes
packages/flutter_driver/lib/src/common/diagnostics_tree.dart Outdated
Copy link
Contributor

@tvolkert tvolkert Jun 14, 2019

Choose a reason for hiding this comment

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

Says that it defaults to zero, but it looks like it can be null above?

Copy link
Member Author

@goderbauer goderbauer Jun 17, 2019

Choose a reason for hiding this comment

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

That null handling as a left-over from something else I tried. I am going to remove it and add a non-null assert.

tvolkert
tvolkert reviewed Jun 14, 2019
View reviewed changes
packages/flutter_driver/lib/src/common/diagnostics_tree.dart Outdated
Copy link
Contributor

@tvolkert tvolkert Jun 14, 2019

Choose a reason for hiding this comment

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

NPE if subtreeDepth is null

Copy link
Member Author

@goderbauer goderbauer Jun 17, 2019

Choose a reason for hiding this comment

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

Calling toString on null is actually perfectly legal: https://dartpad.dartlang.org/be63aa1861c7e7c1dccfbd159121dba0 :)

tvolkert
tvolkert approved these changes Jun 14, 2019
View reviewed changes
Copy link
Contributor

@tvolkert tvolkert left a comment

Choose a reason for hiding this comment

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

LGTM

@goderbauer goderbauer merged commit 4f5d901 into flutter:master Jun 19, 2019
@goderbauer goderbauer deleted the diagdriver branch June 19, 2019 01:31
johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019
@goderbauer @johnsonmh
Add Driver command to get diagnostics tree (flutter#34440)
47b20be
@jacek-marchwicki jacek-marchwicki mentioned this pull request Dec 3, 2019
Merged
7 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@adazh adazh adazh left review comments

@dnfield dnfield dnfield approved these changes

@tvolkert tvolkert tvolkert approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: tests "flutter test", flutter_test, or one of our tests customer: espresso framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Provide testing extension API that matches widgets and returns matched widget information

6 participants

@goderbauer @dnfield @tvolkert @adazh @googlebot @Piinks