Skip to content

Flutter Driver - Create widget finders from serialized finders extensions#67456

Merged
fluttergithubbot merged 13 commits intoflutter:masterfrom
marcin-jelenski:master
Oct 8, 2020
Merged

Flutter Driver - Create widget finders from serialized finders extensions#67456
fluttergithubbot merged 13 commits intoflutter:masterfrom
marcin-jelenski:master

Conversation

@marcin-jelenski
Copy link
Contributor

@marcin-jelenski marcin-jelenski commented Oct 6, 2020 •
edited
Loading

Description

Pull Request #64308 introduced extensions for flutter driver finders. It was incomplete, as the driver isn't really creating finders. This PR solves the issue by calling FinderExtension::createFinder.

Related Issues

#64308

Tests

  • packages/flutter_driver/test/src/real_tests/extension_test.dart

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.

@flutter-dashboard flutter-dashboard bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Oct 6, 2020
@flutter-dashboard
Copy link

flutter-dashboard bot commented Oct 6, 2020

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.

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

@marcin-jelenski
Copy link
Contributor Author

marcin-jelenski commented Oct 6, 2020

CC @Im-Kevin @dnfield

@goderbauer goderbauer requested a review from dnfield October 7, 2020 21:40
dnfield
dnfield reviewed Oct 7, 2020
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.

This seems reasonable, but needs some test(s) that would have failed without this change.

@marcin-jelenski
Copy link
Contributor Author

marcin-jelenski commented Oct 8, 2020

@dnfield thank you for the review. Tests and new lines added.

dnfield
dnfield reviewed Oct 8, 2020
View reviewed changes
dnfield
dnfield reviewed Oct 8, 2020
View reviewed changes
dnfield
dnfield reviewed Oct 8, 2020
View reviewed changes
dnfield
dnfield reviewed Oct 8, 2020
View reviewed changes
dnfield
dnfield reviewed Oct 8, 2020
View reviewed changes
dnfield
dnfield reviewed Oct 8, 2020
View reviewed changes
dnfield
dnfield reviewed Oct 8, 2020
View reviewed changes
dnfield
dnfield reviewed Oct 8, 2020
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.

Mostly style nits.

We don't use dartfmt here. If you wan tto try to use it, make sure to at least increase the line length limit to something like 120, and that should get you pretty close to flutter style.

dnfield
dnfield approved these changes Oct 8, 2020
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

@marcin-jelenski
Copy link
Contributor Author

marcin-jelenski commented Oct 8, 2020

@dnfield updated formatting and changed test variable names.

@dnfield
Copy link
Contributor

dnfield commented Oct 8, 2020

There are some files with trailing whitespace that need to be fixed.

@marcin-jelenski
Copy link
Contributor Author

marcin-jelenski commented Oct 8, 2020

@dnfield I've noticed just one file with unnecessary trailing spaces. Fixed.

@dnfield
Copy link
Contributor

dnfield commented Oct 8, 2020

Yes, sorry, it was just one file :) It shows up in the analyze task(s).

@fluttergithubbot fluttergithubbot merged commit 74f6fa4 into flutter:master Oct 8, 2020
jonahwilliams pushed a commit that referenced this pull request Oct 8, 2020
Revert "Flutter Driver - Create widget finders from serialized finder…
ae562bc 
…s extensions (#67456)"

This reverts commit 74f6fa4.
@jonahwilliams jonahwilliams mentioned this pull request Oct 8, 2020
Merged
jonahwilliams pushed a commit that referenced this pull request Oct 8, 2020
Revert "Flutter Driver - Create widget finders from serialized finder…
4042eb9 
…s extensions (#67456)" (#67687)

This reverts commit 74f6fa4.
@marcin-jelenski marcin-jelenski mentioned this pull request Oct 9, 2020
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dnfield dnfield Awaiting requested review from dnfield

Assignees

No one assigned

Labels

a: tests "flutter test", flutter_test, or one of our tests 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.

4 participants

@marcin-jelenski @dnfield @googlebot @fluttergithubbot