-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Add Windows performance benchmark #99564
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
Add Windows performance benchmark #99564
Conversation
.ci.yaml
Outdated
| - bin/** | ||
| - .ci.yaml | ||
|
|
||
| - name: Windows home_scroll_perf__timeline_summary |
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.
Builder name follows the format Platform + task name: Windows home_scroll_perf__timeline_summary => Windows windows_home_scroll_perf__timeline_summary
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.
Not sure if this will fail the led run, will try based on this PR to see how it goes.
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.
Fixed
|
Updated the tool so it now supports non-fatal --screenshot on unsupported devices |
| Cache.enableLocking(); | ||
| }); | ||
|
|
||
| testUsingContext('warns if screenshot is not supported but continues test', () async { |
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.
FYI @christopherfujino , this is a lot easier than tracking whether each device supports screenshotting in the devicelab codebase
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.
SGTM
keyonghan
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.
We also need an entry in ownership file: https://github.com/flutter/flutter/blob/master/TESTOWNERS to bypass the owner check.
.ci.yaml
Outdated
|
|
||
| - name: Windows windows_home_scroll_perf__timeline_summary | ||
| recipe: devicelab/devicelab_drone | ||
| presubmit: true |
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 current workflow is using bringup: true first.
We don't need presubmit: true here: 1) ci yaml roller use default value true for presubmit 2) With this set, it will cause the presubmit hang on this test as there is not available builder config yet.
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.
Done
TESTOWNERS
Outdated
| ## Windows Devicelab tests | ||
| /dev/devicelab/bin/tasks/windows_home_scroll_perf__timeline_summary.dart @jonahwilliams @flutter/engine | ||
|
|
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: this can be added in section: ## Host only DeviceLab 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.
Done
keyonghan
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.
LGTM re CI config.
dnfield
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.
Test LGTM
christopherfujino
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.
LGTM
| Cache.enableLocking(); | ||
| }); | ||
|
|
||
| testUsingContext('warns if screenshot is not supported but continues test', () async { |
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.
SGTM
…presubmit (#125343) Introduced in #99564. Checked with @jonahwilliams and this should not be running in presubmit.
We need at least one benchmark to test if flutter/engine#31830 improves rendering performance.
This adds a single windows host benchmark, with a small change to the tool to simplify devicelab integration