Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[integration_test] Recommend tests to be in integration_test/, fix example #2986

Merged
merged 4 commits into from
Sep 12, 2020

Conversation

jiahaog
Copy link
Member

@jiahaog jiahaog commented Sep 1, 2020

Description

In this way, there is a clear distinction between integration tests that run on a device (in integration_test/, and widget tests that run with the flutter tester in test/.

Related Issues

flutter/flutter#64690

@jiahaog jiahaog requested a review from dnfield as a code owner September 1, 2020 03:41
jiahaog added a commit to jiahaog/flutter-plugin_tools that referenced this pull request Sep 1, 2020
In flutter/plugins#2986, and flutter/flutter#64690, we want to drop the `_e2e.dart` suffix from test code, and tell users to place them in the `integration_test/` directory. This PR adds support so that tests in this directory will run on FTL.
jiahaog added a commit to jiahaog/flutter-plugin_tools that referenced this pull request Sep 1, 2020
In flutter/plugins#2986, and flutter/flutter#64690, we want to drop the `_e2e.dart` suffix from test code, and tell users to place them in the `integration_test/` directory. This PR adds support so that tests in this directory will run on FTL.
…example

In this way, there is a clear distinction between integration tests that run on a device (in `integration_test/`, and widget tests that run with the flutter tester in `test/`.

flutter/flutter#64690
@jiahaog jiahaog force-pushed the integration-test-dir branch from 4907805 to 9e01841 Compare September 1, 2020 04:28
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

@nturgut @ditman any input on this?

Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

Really small correction about integration tests not being supported in dev mode in web, and about the structure of web plugins.


Invoke `IntegrationTestWidgetsFlutterBinding.ensureInitialized()` at the start
of a test file, e.g.
Create a `integration_test/` directory for your package. In this directory,
Copy link
Contributor

Choose a reason for hiding this comment

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

In the cirrus file we are still using test_driver/ directory even though it is not the recommended directory?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry, I just read the recommended package structure.

Is there a reason for these recommendations? One idea that comes to my mind is: integration_test_driver.dart won't be the same for different tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

It won't be the same but I would think for most use cases the bulk of the testing code should go into the test script that runs on the device, and the driver is mainly for reporting.

Currently, some packages place the test scripts in test/foo_e2e.dart or test_driver/foo_e2e.dart. This works today, because of the old e2e naming, but if they were to rename their tests to like test/foo_integration_test.dart and test_driver/foo_integration_test.dart, this would conflict with globbing rules today that check for the *_test.dart suffix, hence the recommendation of a new integration_test/ directory for such tests

Copy link
Contributor

Choose a reason for hiding this comment

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

We did the renaming like this last week in the for the web engine: flutter/engine#20954

@nturgut
Copy link
Contributor

nturgut commented Sep 8, 2020

I think this approach blocks the possibilities that can be tested with e2e tests. Let me explain more in details:

For example in the extended test example we are creating a driver and give it to the integrationDriver method. We also give a handler for handling the screenshots.

This way, depending on the repository, developers can handle the screenshots differently. Even better, FlutterDriver can be created differently, for example a WebDriver can enable accessibility in advance. Or FlutterDriver can be provided with different options for the driver.

Also, the same application, can use different driver files since they can provide different functionalities.

Due to this reasons, we will probably won't switch to this usage in web integration tests on the engine side. I also recommend keeping the other option in the documentation for power users who wants to provide a wider set of tests.

@jiahaog
Copy link
Member Author

jiahaog commented Sep 9, 2020

@nturgut Sorry, I might have missed something. Can you elaborate more about how this approach blocks certain testing use cases? Customizing the driver is still possible with a custom driver file – in this PR, I have moved the extended test example to follow this proposed structure. There is a change to the command used to kick off the tests, but there is no change in functionality.

@nturgut
Copy link
Contributor

nturgut commented Sep 9, 2020

@nturgut Sorry, I might have missed something. Can you elaborate more about how this approach blocks certain testing use cases? Customizing the driver is still possible with a custom driver file – in this PR, I have moved the extended test example to follow this proposed structure. There is a change to the command used to kick off the tests, but there is no change in functionality.

Sorry, I might be missing something, the README suggestion is:

An accompanying driver script will be needed that can be shared across all
integration tests. Create a `integration_test_driver.dart` in the `test_driver/`
directory with the following contents:

This suggestion might hide other possible usages. For example we can add:

  • a note saying multiple driver files can be used
  • we can also add an example of how we can pass a FlutterDriver

In my opinion, if developer adds multiple driver files, then the advantage of the new structure is not very clear:

lib/
  ...
integration_test/
  foo_test.dart
  bar_test.dart
test/
  # Other unit tests go here.
test_driver/
  integration_test_driver.dart
  integration_test_driver_with_screenshot.dart
  integration_test_driver_with_cstm_setup.dart

@jiahaog
Copy link
Member Author

jiahaog commented Sep 10, 2020

Thanks for clarifying – you're right that I wasn't clear enough in how we can use multiple driver files, I have updated the PR and linked it to the extended driver script.

My understanding is that one of the goals for package:integration_test is to push as much logic as possible into the test script, so that the test will be compatible with tools that do not provide access to the host, like Firebase Test Lab. I have not been exposed too much to web use cases, so let me know if I'm wrong, but I was going with the assumption that customizing the driver script should be only done rarely (such as to pull artifacts from the device), and there should be a significantly larger number of test scripts that run on the device to driver scripts which this recommendation will benefit. Taking a brief look at the e2etests/ directory of the engine, I see that most of the driver scripts are delegating to integrationDriver, so sharing the same driver script might be helpful there.

@jiahaog jiahaog requested a review from nturgut September 10, 2020 07:46
Copy link
Contributor

@nturgut nturgut left a comment

Choose a reason for hiding this comment

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

Thanks for updating the documentation.

@jiahaog
Copy link
Member Author

jiahaog commented Sep 12, 2020

Thanks!

@jiahaog jiahaog merged commit fd6d45c into flutter:master Sep 12, 2020
@jiahaog jiahaog deleted the integration-test-dir branch September 12, 2020 02:25
danielroek pushed a commit to Baseflow/flutter-plugins that referenced this pull request Sep 18, 2020
…example (flutter#2986)

* [integration_test] Recommend tests to be in `integration_test/`, fix example

In this way, there is a clear distinction between integration tests that run on a device (in `integration_test/`, and widget tests that run with the flutter tester in `test/`.

Fix flutter/flutter#64690
jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020
…example (flutter#2986)

* [integration_test] Recommend tests to be in `integration_test/`, fix example

In this way, there is a clear distinction between integration tests that run on a device (in `integration_test/`, and widget tests that run with the flutter tester in `test/`.

Fix flutter/flutter#64690
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
…example (flutter#2986)

* [integration_test] Recommend tests to be in `integration_test/`, fix example

In this way, there is a clear distinction between integration tests that run on a device (in `integration_test/`, and widget tests that run with the flutter tester in `test/`.

Fix flutter/flutter#64690
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants