-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[e2e] Use new FrameTiming constructor. #2914
Conversation
A new FrameTiming constructor was introduced here: flutter/engine@409a5e5, so we need to update the test for it to run in newer versions of flutter.
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.
Thank you for the updates! I should have updated this.
The newest version also require a vsyncStart
field.
@@ -16,7 +16,12 @@ void main() { | |||
rasterTimes = rasterTimes.reversed.toList(); | |||
List<FrameTiming> inputData = <FrameTiming>[ | |||
for (int i = 0; i < 100; i += 1) | |||
FrameTiming(<int>[0, buildTimes[i], 500, rasterTimes[i]]), | |||
FrameTiming( | |||
buildStart: 0, |
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 for the updates!
The new constructor introduced vsyncStart
. Please use the following (or remove that 50+
and change the following expect
accordingly:
FrameTiming(
vsyncStart: 0,
buildStart: 50,
buildFinish: 50+buildTimes[i],
rasterStart: 500,
rasterFinish: rasterTimes[i],
),
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've added vsyncStart: 0 and buildStart: 0. For now the object that this code is testing doesn't seem to use the vsyncStart. I've left a TODO, linked to this issue in the code to revisit this.
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.
Sounds good. I can handle this TODO.
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
We should unbreak this upstream |
I expect this to break |
This feature doesn't have to be in e2e: we were considering it somewhere in the framework or the engine but was thinking that e2e is most relevant. But it seems that this is a very strong argument against this decision. |
Tests appear to be vvvvery slow. I'll close this, because this is not the fix we need. |
Description
A new FrameTiming constructor was introduced here:
flutter/engine@409a5e5, and now one of the tests in the e2e plugin doesn't compile anymore.
(See here.)
This change updates the test to use the new API.
Related Issues
flutter/engine#20229
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
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?