`AutomatedTestWidgetsFlutterBinding.pump` provides wrong pump time stamp, probably because of forgetting the precision by fzyzcjy · Pull Request #112609 · flutter/flutter

fzyzcjy · 2022-09-29T02:44:43Z

The fix is just one line:

I have git blame and find the bug exist since the first version 7yr ago, and no special comments about why this is introduced so I guess it is but not feature.

IMHO the bug may be written like, the programmer wants to convert DateTime (the clock.now()) into a Duration. But then it is forgotten that both are microseconds precision instead of milliseconds precision, and the milliseconds approach is used.

List which issues are fixed by this PR. You must list at least one issue.
Close #112610

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

p.s. test fails with old code, confirming that the test is effective.

This reverts commit 857765b.

pdblasi-google · 2022-10-01T00:06:01Z

+  /// In order to let animation get accurate time information during [pump],
+  /// set the flag to true. The default value is false for compatibility.
+  bool accuratePump = false;
+


Is this because of the test failures in your earlier commits?

From what I can tell, one of those failures would be expected (the widget_tester_test.dart "pumping pumpFrames" test), and the other test is a customer test that I think is reasonable to make a change to or temporarily disable. I'll have to follow up with some people on whether the customer test change is acceptable or not.

If getting a change to the customer test is reasonable, I'd much prefer we get rid of this flag, as it doesn't actual do anything except keep backwards compatibility for something we've pretty thoroughly determined is a bug.

I'll followup again on Monday, but I'm hoping we can fix the failing customer test to prevent needing to add this flag.

Yes, I add that flag because CI fails by widget_tester_test.dart and a consumer-test.

and the other test is a customer test that I think is reasonable to make a change to or temporarily disable. I'll have to follow up with some people on whether the customer test change is acceptable or not.

Take your time :)

If getting a change to the customer test is reasonable, I'd much prefer we get rid of this flag, as it doesn't actual do anything except keep backwards compatibility for something we've pretty thoroughly determined is a bug.

Totally agree! I also want to remove the flag

Apologies for the delay, but I've reached out to the owner of the customer test that's failing and we think we can get a fix in the test that's compatible with this change without the flag. I'll work on getting that test fix in, while I'm doing that could you:

Remove the accuratePump flag

Fix the test in the widget_tester_test.dart file that was failing

After we get those failing tests cleared up, this should be good to go!

Alright, the customer test should be fixed now. I tested it with the microsecond change locally to verify that the customer test works before and after this change.

Thanks :) Code done, waiting for CI

This reverts commit decd3c4.

This reverts commit 0b53843.

This reverts commit e0b5882.

This reverts commit 2e2e486.

# Conflicts: # packages/flutter_test/test/bindings_test.dart

fzyzcjy · 2022-10-06T00:16:57Z

| 00:15 +36: /b/s/w/ir/x/t/flutter_customer_testing.flutter_packages.RNUBSQ/tests/packages/animations/test/open_container_test.dart: Container closes - Fade (by default)
| ══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
| The following TestFailure was thrown running a test:
| Expected: 1.0 (±1e-10)
|   Actual: <0.9999833333333332>
|    Which: 0.9999833333333332 is not in the range of 1.0 (±1e-10).
|
| When the exception was thrown, this was the stack:
| #4      main.<anonymous closure> (file:///b/s/w/ir/x/t/flutter_customer_testing.flutter_packages.RNUBSQ/tests/packages/animations/test/open_container_test.dart:273:7)
| <asynchronous suspension>
| <asynchronous suspension>
| (elided one frame from package:stack_trace)
|
| This was caught by the test expectation on the following line:
|   file:///b/s/w/ir/x/t/flutter_customer_testing.flutter_packages.RNUBSQ/tests/packages/animations/test/open_container_test.dart line 273
| The test description was:
|   Container closes - Fade (by default)
| ════════════════════════════════════════════════════════════════════════════════════════════════════
|
| 00:15 +37 -1: /b/s/w/ir/x/t/flutter_customer_testing.flutter_packages.RNUBSQ/tests/packages/animations/test/fade_scale_transition_test.dart: FadeScaleTransitionConfiguration builds a new route
| 00:15 +37 -1: /b/s/w/ir/x/t/flutter_customer_testing.flutter_packages.RNUBSQ/tests/packages/animations/test/open_container_test.dart: Container closes - Fade (by default) [E]
|   Test failed. See exception logs above.
|   The test description was: Container closes - Fade (by default)

fzyzcjy · 2022-10-06T00:17:28Z

@pdblasi-google I guess maybe need to update the custom testing configurations to point to the latest tests?

pdblasi-google · 2022-10-06T18:20:51Z

@fzyzcjy Yup, you called it. Apologies, I forgot to point the flutter/tests repo to the latest tests. PR is up for that now, I'll ping here when it goes through.

flutter-dashboard · 2022-10-06T20:44:21Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #112609 at sha 02ebd54

pdblasi-google

Alright, customer tests are passing, golden files that were affected by the precision change have been triaged and updated, and the code itself looks good to me!

I appreciate your patience working through the tests and everything. I'll pass it off to someone else for another review, and you should be good to go! Good find!

fzyzcjy · 2022-10-06T23:22:52Z

Thanks and 🎉 !

goderbauer

LGTM

… time stamp, probably because of forgetting the precision (flutter/flutter#112609)

CaseyHillers · 2022-10-14T01:40:05Z

@fzyzcjy @goderbauer this is a breaking change. Can a migration guide be written on how developers can migrate their code with this change? I'm not sure what's needed on my end as a developer, and my animation tests are now very flaky.

fzyzcjy · 2022-10-14T01:44:06Z

@CaseyHillers Hi,

Can a migration guide be written on how developers can migrate their code with this change? ...and my animation tests are now very flaky.

Could you please share some flaky test minimal reproducible samples? IMHO this should not make any problem so want to see reproductions in order to know what happens

CaseyHillers · 2022-10-14T01:57:31Z

Here's an example now that is flaky:

      await tester.pumpWidget(myAnimatedWidget);
      await tester.pumpAndSettle();

      await tester.sendSelectEvent();

      await tester.pumpFrames(scene, Duration(milliseconds: 100));
      await expectLater(
          find.byType(MyAnimatedWidget),
          matchesGoldenFile(
              'animated_widget'));

The resulting goldens are changing. When I change pumpFrames to microseconds, I am still seeing the same flakiness. I'm unsure if it's because the earlier clocks are still in millisecond mode.

fzyzcjy · 2022-10-14T02:00:52Z

@CaseyHillers Weird. Some possible ideas:

Do you have anything that depends on e.g. a real clock? If so, it will be flaky. (I guess no)
Could you please change MyAnimatedWidget to something like AnimatedBuilder(builder: (_, value) => Text('the value is: $value'). Then, when the golden is changing, we can know what value indeed it is having.

I'm unsure if it's because the earlier clocks are still in millisecond mode.

Do you mean the new golden (i.e. after the PR) are different from old golden, and the new golden is itself stable? If so, looks like it is possible. Indeed the animation controller is fed with a changed animation time.

CaseyHillers · 2022-10-14T02:22:06Z

My animated widgets are just an animated builder that has a custom animation controller. @goderbauer or @pdblasi-google can help with reproducing the issue here.

My understanding is I need to change every possible clock to be in microseconds instead of milliseconds. This seems to be a breaking change for any other customers, and I'm having a difficult time tracking all the various clocks in my codebase.

I haven't found the discord threads, but I wonder if this is something that should be in the framework. There could be a tester field added for high precision or this can be added directly to your package. I assume most Flutter tests aren't needing high precision, and this is going to cause a lot of pain once it's in beta/stable.

fzyzcjy · 2022-10-14T02:24:29Z

@CaseyHillers

I need to change every possible clock to be in microseconds instead of milliseconds

Btw I am curious why a clock can in milliseconds in codebase - clock package, Duration, DateTime, etc are all in microseconds.

I haven't found the discord threads, but I wonder if this is something that should be in the framework. There could be a tester field added for high precision or this can be added directly to your package. I assume most Flutter tests aren't needing high precision, and this is going to cause a lot of pain once it's in beta/stable.

I agree that an alternative solution is to use a bool flag to enable high-accuracy (I have done that indeed - e0b5882).

… time stamp, probably because of forgetting the precision (#112609)" This reverts commit 1966aaf.

CaseyHillers · 2022-10-14T11:38:10Z

I agree that an alternative solution is to use a bool flag to enable high-accuracy (I have done that indeed - e0b5882).

Thanks, that sounds like it would work for me! Are there plans to upstream that change?

… time stamp, probably because of forgetting the precision (flutter#112609)" This reverts commit 1966aaf.

fzyzcjy · 2022-10-14T11:39:13Z

@CaseyHillers I am ok with that, just not sure what other reviewers think? (Since that was my original design and later a reviewer suggests me to change to what is current merged.) If you guys are OK I will PR it.

… wrong pump time stamp, probably because of forgetting the precision (#112609)" (#113432) This reverts commit 1966aaf.

fzyzcjy · 2022-10-14T11:48:14Z

@CaseyHillers Here is the PR: #113433

… wrong pump time stamp, probably because of forgetting the precision (#112609)" (#113432) (#113557) This reverts commit 1966aaf. Co-authored-by: Casey Hillers <[email protected]>

fzyzcjy added 2 commits September 29, 2022 10:34

fix precision issue

a0a8002

add tests

1009a0d

flutter-dashboard Bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Sep 29, 2022

This was referenced Sep 29, 2022

AutomatedTestWidgetsFlutterBinding.pump should match engine frame precision #112610

Closed

AutomatedTestWidgetsFlutterBinding.pump does not provide real timestamp to handleBeginFrame, but clamp at milliseconds fzyzcjy/flutter_smooth#48

Closed

fix test error

857765b

fzyzcjy mentioned this pull request Sep 29, 2022

[animations] Let test time be accurate (originally has 1us shift) flutter/packages#2657

Closed

9 of 11 tasks

fzyzcjy added 3 commits September 29, 2022 11:41

Revert "fix test error"

2e2e486

This reverts commit 857765b.

add and use flag accuratePump

e0b5882

fix doc error

0b53843

fzyzcjy marked this pull request as ready for review September 29, 2022 05:11

fzyzcjy added 2 commits September 29, 2022 14:11

Merge branch 'flutter:master' into feat/pump

6af9510

Update binding.dart

decd3c4

HansMuller requested a review from pdblasi-google September 30, 2022 22:01

pdblasi-google reviewed Oct 1, 2022

View reviewed changes

fzyzcjy added 5 commits October 6, 2022 07:13

Revert "Update binding.dart"

c0ce8c4

This reverts commit decd3c4.

Revert "fix doc error"

4414904

This reverts commit 0b53843.

Revert "add and use flag accuratePump"

4634dd2

This reverts commit e0b5882.

Revert "Revert "fix test error""

38d1ceb

This reverts commit 2e2e486.

Merge branch 'master' into feat/pump

02ebd54

# Conflicts: # packages/flutter_test/test/bindings_test.dart

flutter-dashboard Bot added the will affect goldens Changes to golden files label Oct 6, 2022

pdblasi-google approved these changes Oct 6, 2022

View reviewed changes

pdblasi-google requested a review from goderbauer October 6, 2022 22:48

goderbauer approved these changes Oct 10, 2022

View reviewed changes

goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 10, 2022

auto-submit Bot merged commit 1966aaf into flutter:master Oct 10, 2022

This was referenced Oct 11, 2022

Roll Flutter from 47fc19314946 to 4fed6aaeff8f (19 revisions) flutter/packages#2702

Merged

Roll Flutter from 89d9880f46e0 to 4fed6aaeff8f (23 revisions) flutter-team-archive/plugins#6564

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 11, 2022

1966aaf AutomatedTestWidgetsFlutterBinding.pump provides wrong pump…

19b165b

… time stamp, probably because of forgetting the precision (flutter/flutter#112609)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Oct 11, 2022

1966aaf AutomatedTestWidgetsFlutterBinding.pump provides wrong pump…

ab60f99

… time stamp, probably because of forgetting the precision (flutter/flutter#112609)

XilaiZhang mentioned this pull request Oct 14, 2022

Revert "AutomatedTestWidgetsFlutterBinding.pump provides wrong pump time stamp, probably because of forgetting the precision" #113415

Merged

XilaiZhang added a commit that referenced this pull request Oct 14, 2022

Revert "AutomatedTestWidgetsFlutterBinding.pump provides wrong pump…

dc1c537

… time stamp, probably because of forgetting the precision (#112609)" This reverts commit 1966aaf.

CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request Oct 14, 2022

Revert "AutomatedTestWidgetsFlutterBinding.pump provides wrong pump…

85aa38d

… time stamp, probably because of forgetting the precision (flutter#112609)" This reverts commit 1966aaf.

CaseyHillers pushed a commit that referenced this pull request Oct 14, 2022

[releases] Revert "AutomatedTestWidgetsFlutterBinding.pump provides…

5f2f2de

… wrong pump time stamp, probably because of forgetting the precision (#112609)" (#113432) This reverts commit 1966aaf.

fzyzcjy mentioned this pull request Oct 14, 2022

Reland AutomatedTestWidgetsFlutterBinding.pump provides wrong pump time stamp, probably because of forgetting the precision, via optional flag #113433

Closed

7 of 8 tasks

itsjustkevin mentioned this pull request Oct 17, 2022

[releases] Revert "AutomatedTestWidgetsFlutterBinding.pump provides… #113557

Merged

Conversation

fzyzcjy commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

pdblasi-google Oct 1, 2022

Choose a reason for hiding this comment

Uh oh!

pdblasi-google Oct 1, 2022

Choose a reason for hiding this comment

Uh oh!

fzyzcjy Oct 1, 2022

Choose a reason for hiding this comment

Uh oh!

pdblasi-google Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

pdblasi-google Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

fzyzcjy Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

fzyzcjy commented Oct 6, 2022

Uh oh!

fzyzcjy commented Oct 6, 2022

Uh oh!

pdblasi-google commented Oct 6, 2022

Uh oh!

flutter-dashboard Bot commented Oct 6, 2022

Uh oh!

pdblasi-google left a comment

Choose a reason for hiding this comment

Uh oh!

fzyzcjy commented Oct 6, 2022

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

CaseyHillers commented Oct 14, 2022

Uh oh!

fzyzcjy commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CaseyHillers commented Oct 14, 2022

Uh oh!

fzyzcjy commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CaseyHillers commented Oct 14, 2022

Uh oh!

fzyzcjy commented Oct 14, 2022

Uh oh!

CaseyHillers commented Oct 14, 2022

Uh oh!

fzyzcjy commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fzyzcjy commented Oct 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fzyzcjy commented Sep 29, 2022 •

edited

Loading

fzyzcjy commented Oct 14, 2022 •

edited

Loading

fzyzcjy commented Oct 14, 2022 •

edited

Loading

fzyzcjy commented Oct 14, 2022 •

edited

Loading