[go: up one dir, main page]

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

Integration testing recipes #1128

Merged
merged 4 commits into from
Aug 16, 2018
Merged

Conversation

brianegan
Copy link
Contributor
@brianegan brianegan commented Aug 2, 2018

Hey hey :)

This adds to the other recipes on testing! The "Test your apps" page has a decent summary of writing integration tests, but I felt like it was missing a lot of important details when I first started writing integration tests. Therefore, I split that example up into smaller chunks and tried to provide a bit more detail for each part.

Three recipes included:

  • Introduction to integration testing
  • Scrolling in integration tests
  • Performance profiling with integration tests

Guidance needed: Should I change up the "Test your apps" page a bit as well? My idea: the page provides a great overview of the testing solutions that Flutter provides. I was thinking of keeping that overview and removing the examples, linking to all of these recipes instead. Thoughts?

@googlebot googlebot added the cla: yes Contributor has signed the Contributor License Agreement label Aug 2, 2018
@Sfshaza
Copy link
Contributor
Sfshaza commented Aug 3, 2018

This looks amazing, @brianegan! I am OOO today, so will ask for some engineering review.

@chalin, @chinmaygarde

@Sfshaza
Copy link
Contributor
Sfshaza commented Aug 11, 2018

@yjbanov, would you mind reviewing?

Copy link
Contributor
@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

Wow, these are fantastic docs! I like the breakdown into integration test basics, scrolling and perf testing. LGTM with a few nitpicks.

Unit tests and Widget tests are handy for testing individual classes, functions,
or Widgets. However, they generally don't test how individual pieces work
together as a whole or capture the performance of an application running on a
real device. These tasks are performed with integration tests.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I don't know if we use this style in other parts of our docs, but I like when first mentions of important terms are made italic, e.g. integration tests.

}
```

### 2. Add the `flutter_test` dependency
Copy link
Contributor

Choose a reason for hiding this comment

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

flutter_driver

Then, run the following command from the root of the project:

```
flutter drive --target=test_driver/app.dart
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth mentioning that if the name of the driver script is something other than app.dart that it can be specified using the --driver option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Sfshaza What are your thoughts here? I could definitely add that info, but I think I've seen other PRs where it's recommended that we don't dive too deeply into the different options the CLI tools provide.

The `Timeline` object provides detailed information about all of the events that
took place. Therefore, it can be a bit difficult to read and understand.

Instead of inspecting the raw data contained within the `Timeline`, we can
Copy link
Contributor

Choose a reason for hiding this comment

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

There a second option: https://docs.flutter.io/flutter/flutter_driver/TimelineSummary/writeTimelineToFile.html

This method will write the full timeline to a file. The file can then be opened in the chrome://tracing, which provides a nice UI for manually inspecting the timeline. It is useful when you need to analyze a performance regression in a specific test.

Copy link
Contributor Author
@brianegan brianegan Aug 14, 2018

Choose a reason for hiding this comment

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

Oh wow, I had no idea you could import that into Chrome's tracing tools! I'll definitely add that info.

method allows us to scroll through a specific list by a given amount.
- The
[`scrollIntoView`](https://docs.flutter.io/flutter/flutter_driver/FlutterDriver/scrollIntoView.html)
method finds a specific Widget that's currently displayed on screen and will
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "currently displayed on screen" may not be accurate. It may not actually be visible on the screen. It might be rendered offscreen (e.g. clipped by a scrollable viewport). However, the widget we're looking for must be instantiated and rendered into the render object hierarchy, even if its pixels are not painted on screen.

This method is useful when you know that something is eagerly instantiated and all you want is shift to a position such that it's visible. Perhaps you are scrolling a SingleChildScrollView. Or perhaps you are testing a scrollable container that renders widgets on-demand (e.g. ListView.builder) and you expect that at a certain position a certain widget must currently be rendered.

Contrast this to scrollUntilVisible, which will scroll "hoping" that eventually the widget we are looking for will be rendered. While scrollIntoView will fail instantly if it's not already there, scrollUntilVisible may keep scrolling hopelessly until it times out (especially if the list is indefinite).

Suggestion: perhaps instead of "displayed" or "visible", we say "rendered", which strongly implies RenderObject. A paragraph explaining the difference between being "rendered" and being "visible" might help readers better understand how these methods work (and the framework in general; scrolling is complicated 😅).

Copy link
Contributor Author
@brianegan brianegan Aug 14, 2018

Choose a reason for hiding this comment

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

Good call -- I'll definitely add some more info here since, as you can tell, I was a bit confused by looking at the API docs on exactly how that was working :P

Would it be ok to ping you for a review once I add it? I definitely want to make sure everything is technically correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course! Also, if you can come up with a better wording for our dartdocs, please feel free to send a PR to improve it.

on screen. In order to verify our apps work on a broad range of devices, we
might run our integration tests against devices with different screen sizes.
Whether or not a particular Widget is visible can depend on the size of the
screen.
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this recommendation, but as explained above "visible on screen" may not be accurate enough.

@brianegan
Copy link
Contributor Author

@yjbanov Thanks so much for the thorough review! Really cool to have your input on this topic.

@brianegan
Copy link
Contributor Author
brianegan commented Aug 15, 2018

@yjbanov I've pushed one more commit! I decided to forego an entire paragraph on the difference between visible vs rendered since I felt it distracted a bit too much from the main point. Instead, I tried to describe how using a ListView.builder will render widgets on-demand and why that could cause problems. Please let me know if these changes are technically accurate and convey the information you'd like to see!

@brianegan
Copy link
Contributor Author
brianegan commented Aug 15, 2018

@Sfshaza The CI is failing because chrome://tracing is not a valid link. Fair enough, it's not universal to all browsers! However, I think it'd be cool to make it a proper link to convey the intent that you need to navigate to that URL in Chrome. What would your advice be in this case? Remove the link, or is there a way to ignore that check for specific links?

@chalin
Copy link
Contributor
chalin commented Aug 15, 2018

... is there a way to ignore that check for specific links?

Yes: add a regexp to the options.url_ignore list in the website/Rakefile.

@brianegan
Copy link
Contributor Author

Thanks @chalin! That did the trick.

@yjbanov
Copy link
Contributor
yjbanov commented Aug 16, 2018

lgtm

@Sfshaza
Copy link
Contributor
Sfshaza commented Aug 16, 2018

Thanks so much, @brianegan!

@Sfshaza
Copy link
Contributor
Sfshaza commented Aug 16, 2018

And thanks for reviewing, @yjbanov!

@Sfshaza Sfshaza merged commit 3e479b3 into flutter:master Aug 16, 2018
@brianegan brianegan deleted the integration-testing branch August 16, 2018 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Contributor has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants