-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Conversation
472f1fb
to
e9bfc8e
Compare
This looks amazing, @brianegan! I am OOO today, so will ask for some engineering review. |
@yjbanov, would you mind reviewing? |
There was a problem hiding this 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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 😅).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
@yjbanov Thanks so much for the thorough review! Really cool to have your input on this topic. |
eac6316
to
af35cd8
Compare
@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 |
@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? |
Yes: add a regexp to the |
14e6c40
to
ee8df07
Compare
Thanks @chalin! That did the trick. |
Thanks so much, @brianegan! |
And thanks for reviewing, @yjbanov! |
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:
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?