Skip to content

Flutter Driver: command extensions and extension feature cleanup #67916

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.

Sign up for GitHub

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

Jump to bottom
Merged
dnfield merged 31 commits into from
Oct 17, 2020
Merged

Flutter Driver: command extensions and extension feature cleanup #67916

dnfield merged 31 commits into from
Oct 17, 2020

Conversation

@marcin-jelenski
Copy link
Contributor

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

Description

As discussed in #62299, ability to reuse and extend flutter driver commands is the very last step to leverage remote execution tools like Appium, Selenium.

Grounds:

  • It was possible to extend Commands and CommandWithTargets, but it wasn't possible to interact with the flutter app from those extensions.
  • Private members and newly created factories were repackaged for the consistent codebase.
  • Documentation for finder extensions was missing explanations and references.

It is not a breaking change, as the moved imports were not accessible on any flutter branch due to a bug (#67711)

Related Issues

#62299
#67711

User issues which could be solved using extensions:
#62489
#57792
#52940
#28753
#50626
#47448
#12810

Tests

I added the following tests:

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

Tests for command extensions.

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

  • 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 12, 2020
@google-cla google-cla bot added the cla: yes label Oct 12, 2020
@marcin-jelenski
Copy link
Contributor Author

marcin-jelenski commented Oct 12, 2020 •
edited
Loading

CC @dnfield @jonahwilliams @goderbauer

This was referenced Oct 12, 2020
Closed
Closed
@marcin-jelenski
Copy link
Contributor Author

marcin-jelenski commented Oct 13, 2020

Added related issues:

#62489
#57792
#52940
#28753
#50626
#47448
#12810

@marcin-jelenski
Copy link
Contributor Author

marcin-jelenski commented Oct 14, 2020 •
edited
Loading

@dnfield @goderbauer would you mind assigning someone who can make a review and decide about moving it forward? We heavily depend on this feature. Thanks.

@dnfield dnfield self-requested a review October 14, 2020 17:34
@dnfield
Copy link
Contributor

dnfield commented Oct 14, 2020

This has been on my queue, just forgot to assign. Sorry about that.

dnfield
dnfield reviewed Oct 16, 2020
View reviewed changes
dnfield
dnfield reviewed Oct 16, 2020
View reviewed changes
dnfield
dnfield reviewed Oct 16, 2020
View reviewed changes
dnfield
dnfield reviewed Oct 16, 2020
View reviewed changes
@marcin-jelenski
Copy link
Contributor Author

marcin-jelenski commented Oct 16, 2020

@dnfield thanks for the review. I've applied all suggestions and would like to ask for a re-review.

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

Just a couple more doc nits :)

@marcin-jelenski
Copy link
Contributor Author

marcin-jelenski commented Oct 16, 2020 •
edited
Loading

@dnfield I've fixed whitespace characters and applied documentation suggestions. Is that a proper use of See also block, you've mentioned?

Also, I've made a minor change - waitForElement and waitForAbsentElement functions can be exposed for usage within extensions:
66196a8

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

@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Oct 16, 2020

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux web_e2e_test has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux web_smoke_test has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Mac build_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

@marcin-jelenski
Copy link
Contributor Author

marcin-jelenski commented Oct 16, 2020 •
edited
Loading

@dnfield automatic checks caught issues with dart:ui imports within examples' e2e tests. Indeed, the code was invalid - Finders were indirectly used in host code.

I've split factories into separate files to avoid conflicts: 00b887c

@dnfield
Copy link
Contributor

dnfield commented Oct 17, 2020

The google testing failure is because this will require some internal build file changes to handle new files. Still LGTM.

@dnfield dnfield merged commit d2d0721 into flutter:master Oct 17, 2020
@marcin-jelenski
Copy link
Contributor Author

marcin-jelenski commented Oct 17, 2020

@dnfield is there any way I can prevent internal testing breakage next time? Thank you for the merge.

@dnfield
Copy link
Contributor

dnfield commented Oct 17, 2020

No, a googler will have to update a build file internally. It's not that big a deal :)

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.

3 participants

@marcin-jelenski @dnfield @fluttergithubbot