@fzyzcjy @CaseyHillers @goderbauer

I still think that this change should be made without the boolean flag. The underlying issue with the goldens is that pumpFrames defines interval with microsecond precision, but the AutomatedTestWidgetsFlutterBinding didn't support microsecond precision. @Piinks and I went over a couple of golden changes in the flutter repos test as well and accepted the changes to those goldens as they were a change to a correct state (which is why the goldens are current hanging for this PR).

The secondary issue that makes this difficult to break cleanly is that LiveWidgetsFlutterBinding does support microsecond precision, so we can't just update pumpFrames' interval parameter to be a clean 16 milliseconds by default, as that would break other tests.

To start with, the first paragraph in the breaking changes process says:

Sometimes, however, doing this is necessary for the greater good. We want our APIs to be intuitive; if being backwards-compatible requires making an API into something that we would never have designed that way unless forced to by circumstances, then we should instead break the API and make it good.

Adding a boolean to make the correct behavior happen is not an API that we would have designed on purpose. It's also not a change that we can easily guide people to using, as there's no "new API" we can drive them towards with deprecations or data driven fixes. It'd be something we introduce for a period of time, hope people read the blog, then end up running into the same "breaking change" issues when we eventually remove the boolean or default it to true.

From there, digging into the process, the preferred process is the three step process:

1. Add new API and opt in to the new API
2. Remove the old API
3. Remove the opt in

I think the only way we can get that to happen is to introduce a new version of pumpFrames that would support the correct behavior, deprecate the current pumpFrames, then eventually remove pumpFrames. Here's what I'd propose:

• Fix AutomatedTestWidgetsFlutterBinding without the flag
• Introduce a new method with the exact contents that pumpFrames currently has:

pumpFramesFor(
  Widget target,
  Duration duration, [
    Duration interval = const Duration(milliseconds: 16, microseconds: 683),
  ])

• Update pumpFrames to pass through to the new pumpFramesFor method:
  • Check if (binding is AutomatedTestWidgetsFlutterBinding)
  • If it is, then truncate the microseconds off of interval before passing through to maintain the current incorrect behavior

My biggest concern with this approach is that pumpFramesFor isn't as clean a name as just pumpFrames, but it's the best I can come up with. Names aside, introducing a new method and deprecating the old is the only way I can think of to keep the existing behavior and actually be able to drive users to the new api before landing the correct behavior.

@pdblasi-google @CaseyHillers @goderbauer I agree with both sides of the opinion, both looks very reasonable to me. So just ping me when googlers reach a conclusion that what I should do!

So... what should I do?

@fzyzcjy

If you're alright with it, I'd like to grab the issue from you to wrap it up. We'll need to make some changes internally and do some extra documentation to release this due to the internal test failures. If you'd like, I'll guide you through the extra docs and just handle the internal stuff myself, but I think it'll be easier with just one person working to get this landed.

@pdblasi-google Sure :) so shall I close this issue now?

This PR yes. I'll still work off of the original issue so we can keep the wonderful history of this surprisingly complex issue! 😛

@pdblasi-google Looking forward to see it landed!