-
Notifications
You must be signed in to change notification settings - Fork 28.8k
Add a E2E based performance test case #61509
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
A few things to address:
Before merge I need to run |
@@ -0,0 +1,192 @@ | |||
// Copyright 2014 The Flutter Authors. All rights reserved. |
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.
This file probably also belongs to test_driver
as this is unlikely to be used as a unit test. BTW, I guess we'll update WidgetTester
to WidgetController
in this file?
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'm not expecting to use that demo very soon but if you think I should, maybe I'd better change this PR back to draft as that demo is far from use-able.
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.
And the reason I'm putting it in test/
rather than test_driver
is that E2E recommends so (https://pub.dev/packages/e2e#test-locations), and the test file (cull_opacity_perf_e2e.dart
) should be able to run using flutter 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.
Changing from WidgetTester
to WidgetController
for driverOps
and setupOps
for now, but this specific case it's not used.
|
||
import 'util.dart'; | ||
|
||
Future<void> main() 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.
This file probably belongs to test_driver
as this is unlikely to be used as a unit 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.
Same reason above. #61509 (comment)
I may also need to update the macrobenchmark/readme.md, which is also the case for #61388. That will be done on whichever of the two PR who lands later. |
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 this is good to go as soon as we minimize the duplicate code to make it a little easier to maintain in the future. Let's try to have this running in our device lab soon :)
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 with nits. CC @dnfield to see if there's any extra comments.
_countExceed(frameRasterizerTimeSorted, kBuildBudget), | ||
); | ||
} | ||
|
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: when a function has a lot of arguments, Flutter usually put them under {}
so it would be like the following
const FrameTimingSummarizer._({
this.frameBuildTime,
...
});
// Use it with argument name provided to reduce confusion
FrameTimingSummarizer._(
...,
averageFrameBuildTime: frameBuildTime.reduce(add) ~/ data.length,
...,
);
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.
{}
means named arguments, and optional if not marked as @required
, with default value if not assigned null
. Here I don't think they should be optional. I added all @required
.
Description
This PR reimplement
cull_opacity_perf__timeline_summary
test ascull_opacity_perf__e2e_summary
using E2E, as a trial for running performance test using E2E that's host-independent in terms of driving the test.Related Issues
This is the first migration of performance test for go/flutter-nonhost
This is also a first step to #38329 for the new test case is self-driven.
Tests
The PR itself is adding a test case
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.