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

enableFlutterDriverExtension: optionally disable text entry emulation #71656

Merged
merged 5 commits into from Dec 16, 2020

Conversation

voobel
Copy link
Contributor

@voobel voobel commented Dec 3, 2020

Description

Added option to enableFlutterDriverExtension so that it can be built without text entry emulation.
Default selection remains as is (enabled text entry emulation).

The change is needed because we want to test also manually the builds that we deploy to test environments. With text entry emulation enabled it is impossible for us to do so.
In tests side having text entry emulation disabled by default causes no harm, as simply running await driver.setTextEntryEmulation(enabled: true); enables it for testing.

In short: it will unblock manual testing with automation builds.

Related Issues

Issue that the change is alleviating/fixing: #9383

Tests

Conformed existing tests in packages/flutter_driver/test/src/real_tests/extension_test.dart and added 2 new tests to cover new parameter behavior:

  • enableTextEntryEmulation false verifies that EnterText fails when enableTextEntryEmulation is set as false
  • enableTextEntryEmulation true verifies that EnterText fails when enableTextEntryEmulation is set as true

Checklist

  • 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.

@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 Dec 3, 2020
@google-cla
Copy link

google-cla bot commented Dec 3, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Dec 3, 2020
@google-cla
Copy link

google-cla bot commented Dec 3, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@voobel
Copy link
Contributor Author

voobel commented Dec 3, 2020

@googlebot I fixed it.

@google-cla google-cla bot added cla: yes and removed cla: no labels Dec 3, 2020
@dnfield
Copy link
Contributor

dnfield commented Dec 9, 2020

Could you consider using package:integration_test instead of this?

That already handles this use case, and going forward that's the library that the team is more focused on maintaining.

@voobel
Copy link
Contributor Author

voobel commented Dec 10, 2020

Could you consider using package:integration_test instead of this?

That already handles this use case, and going forward that's the library that the team is more focused on maintaining.

Sorry, I checked package:integration_test in Github and also documentation and don't really understand how it helps solve the issue that I'm trying to solve.

Note: We're running E2E tests against pre-built .apk/.ipa and pre-deployed web -> without flutter drive (reverse engineered the steps to connect; using flutter packages pub run test to trigger the tests). When using flutter drive (in isolation & from app codebase) then I understand that there's no issue with text entry emulation being enabled by default, as the application is spinned up solely for test execution. Our issue is basically that we want to make pre-built .apk/.ipa and pre-deployed web usable for manual testing aswell (text entry emulation disabled by default && in driver tests explicitly enable).

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.

I think the doc needs a little work but overall this seems fine to me.

FYI @nturgut in case this might have some implication for Web testing that I'm missing.

@nturgut
Copy link
Contributor

nturgut commented Dec 10, 2020

I would like to understand more in details why we needed to enable emulation for text entry. Which platform is this for and what is use case?

Does text entry emulation uses real platform channels?

As Flutter for Web team, text editing is the area that lead us start writing integration tests. So far we haven't had any issues with text entry. I also think enabling emulation for default might lead developers to emulate text entry more. When we were designing e2e tests initially our goal was to provide a testing environment which will test the real (not simulated) code paths as much as possible. Such as real platform channels.

@dnfield
Copy link
Contributor

dnfield commented Dec 10, 2020

My understanding is the goal is to allow developers to build the test application but be able to manually interact with it.

@voobel
Copy link
Contributor Author

voobel commented Dec 11, 2020

I will happily explain this need a bit further:

I would like to understand more in details why we needed to enable emulation for text entry. Which platform is this for and what is use case?
Text entry emulation is enabled by default if to have enableFlutterDriverExtension in app main.dart. This allows FlutterDriver.enterText to successfully enter text to text-input widgets. However, it blocks the manual text entering (in web: keyboard doesn't do anything; in mobile: keyboard doesn't even appear if to tap text-input widget).

Use case of disabling text entry emulation is related to how we test and release our product.
We are building following artifacts on each merge build:

  • debug .apk with enableFlutterDriverExtension
  • debug .ipa with enableFlutterDriverExtension
  • release web with enableFlutterDriverExtension (yes, this is different compared to mobile but it works with release and there's no way (and need?) to build web debug flavor)
  • release .apk without enableFlutterDriverExtension
  • release .ipa without enableFlutterDriverExtension
  • release web without enableFlutterDriverExtension

To all testing environments (dev & staging & validation/pre-live) we deploy versions with enableFlutterDriverExtension enabled to allow automated testing to work. To production we deploy versions without enableFlutterDriverExtension.
Testing environments are also used for manually trying things out, and also the environments are used for demoing purpose. This however requires app to not have text entry emulated, so that text-input widgets can be interacted with. That's the aim of this commit - to get rid of this limitation. In tests side it's no problem to explicitly enable text entry emulation.

The problem applies for both platforms (mobile & web), though it's more painful for web, as for web the application is hosted (no way really to build locally without enableFlutterDriverExtension and then override the version in hosted environment); for mobile one workaround is to build locally without enableFlutterDriverExtension and just install that to device (nothing's hosted - only matters what you install to mobile device).

As you possibly can figure from above, we must do validation against pre-built applications instead of running tests in isolation from app codebase (for ex. by flutter drive). I think it's pretty common "testing & releasing strategy" for many companies.

Does text entry emulation uses real platform channels?

I believe so, excerpt from FlutterDriver.setTextEntryEmulation documentation:

When enabled, the operating system's configured keyboard will not be invoked when the widget is focused, as the SystemChannels.textInput channel will be mocked out.

@nturgut
Copy link
Contributor

nturgut commented Dec 11, 2020

SystemChannels.textInput channel will be mocked out.

Looks like the documentation also clearly says SystemChannels.textInput channel will be mocked out.. This channel is critical to verify communication between framework and the engine. Using a mock for this channel will nullify the benefits of using integration tests. The other problem I see is this statement: configured keyboard will not be invoked. If we consider a test scenario where we are using chrome on android as our testing browser. If we use the mocks, some critical code paths will never be invoked. Such as, example where the onscreen keyboard change the view insets.

If this is critical for your scenario, we can have it as an option, however the value of enableTextEntryEmulation should default to false.

Is there a reason that you initially wanted to set it to true by default? (I might be missing something here so I wanted understand more on the reasoning)

@voobel
Copy link
Contributor Author

voobel commented Dec 14, 2020

@nturgut In current implementation (already before this change) it's true by default (and no way to change it) if to use enableFlutterDriverExtension. I just added option to change it without changing default behavior. I'm not sure we want to change this? Otherwise people would have to start explicitly adding await driver.setTextEntryEmulation(enabled: true); to their driver tests.

Added comment to change that illustrates above-mentioned: https://github.com/flutter/flutter/pull/71656/files#r542194908

If we use the mocks, some critical code paths will never be invoked.

That is how flutter_driver text entry has been designed. I assume there's no other method for this? Not in my powers to change it nor do I know how it "exactly" works.

@nturgut
Copy link
Contributor

nturgut commented Dec 16, 2020

@voobel thanks a lot for the explanation! Looks like I got that part wrong. PR looks good to me then 👍

@voobel voobel requested a review from dnfield December 16, 2020 12:38
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 LGTM.

The flutter_svg failure in customer_testing is because of an engine roll that happened since you opened this PR. I'm going to land this as is because I'm fairly certain about that - if it fails on post we can revert and figure it out.

@dnfield dnfield merged commit ea7017d into flutter:master Dec 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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

3 participants