-
Notifications
You must be signed in to change notification settings - Fork 7.8k
[testing_app] Migrate the sample to the integration_test package #633
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
Conversation
domesticmouse
left a comment
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.
Looks good to me, but I'm deferring to @johnpryan on this one =)
|
@johnpryan, can you give this a look and merge it when you have a chance? |
|
@johnpryan, I would also like to know why did you suggest to follow this directory structure and keep the |
|
It's okay to put the In the integration testing docs we recommend a |
| for (var entry in data.entries) { | ||
| print('Writing ${entry.key} to the disk.'); | ||
| // Default storage destination is the 'build' directory. | ||
| await writeResponseData( |
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.
The docs say that the default responseDataCallback is writeResponseData - is there a reason you are overriding it to write separate files?
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.
For the performance tests, I have recorded the timeline as well as its summary. If I don't manually separate the data into different files, it gets stored into one single file which becomes huge. My current approach also allows the timeline to be opened using Chrome's tracing tools to further evaluate the performance.
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.
You should be able to load a trace into chrome without any additional code here. @dnfield does this sound 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.
The raw trace should be loadable in chrome://tracing.
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.
For some reason, it isn't working for me.
Here's what I did to my current integration-test branch:
- Removed everything passed to
integrationDriver()and kept everything else as it is. - Tried running the perf_test and then loading the
integration_response_data.jsonfile intochrome://tracing.
The file was loaded, but no charts or data appeared.
I also tried removing watchPerformance() to make the reported data contain only timelines and not timeline summaries.
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 think the tracing tools aren't able to load multiple timelines at once. Am I missing something here?
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.
@abd99 Can you file an issue in the Flutter issue tracker?
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.
Sure. I'll create one shortly.
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.
Thanks
Co-authored-by: John Ryan <[email protected]>
Great! I knew that it works both ways, but didn't know if there was a reason behind following one way or the other. |
closes #622
/cc @domesticmouse @redbrogdon @johnpryan
PS - Feel free to take your time and review this PR after you're back from the holidays.