AutomatedTestWidgetsFlutterBinding.pump provides wrong pump time stamp, probably because of forgetting the precision

[fzyzcjy]

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.

[pdblasi-google]

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.

[pdblasi-google]

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

[fzyzcjy]

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

[pdblasi-google]

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:

1. Remove the accuratePump flag
2. 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!

[pdblasi-google]

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.

[fzyzcjy]

Thanks :) Code done, waiting for CI

[fzyzcjy]

| 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]

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

[pdblasi-google]

@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]

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]

Thanks and 🎉 !

[goderbauer]

LGTM

[CaseyHillers]

@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]

@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]

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]

@CaseyHillers Weird. Some possible ideas:

1. Do you have anything that depends on e.g. a real clock? If so, it will be flaky. (I guess no)
2. 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]

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]

@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).

[CaseyHillers]

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?

[fzyzcjy]

@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.

[fzyzcjy]

@CaseyHillers Here is the PR: #113433