flutter / devtools

Performance tools for Flutter
https://flutter.dev/docs/development/tools/devtools/
BSD 3-Clause "New" or "Revised" License
1.58k stars 326 forks source link

Expose ability to pump() and pumpAndSettle() from IDE while debugging widget tests #2150

Open mehmetf opened 4 years ago

mehmetf commented 4 years ago

pump([Duration]) and pumpAndSettle() are poorly understood APIs in Flutter widget testing. Most often, when a test unexpectedly fails, the users simple add pumps until it passes.

One such example we had in Greentea goes like this. Imagine the following API for model layer:

// Sub methods
sub();
unsub();

// Returns null if result is not cached and initiates a "fill" call.
// Returns result if it was cached.
ModelResult getResult({params});

// You then use this in a stateful widget:

initState() => sub();
dispose() => unsub();

Widget build(BuildContext context) {
  final result = getResult(params);
  if (result == null) {
    // Render loading indicator
  } else {
    // Render UI.
  }
}

This is pretty straight forward in a normal execution flow. When it came to testing, we replaced all model services such that the fill call would immediate return a mock result. That however, required an extra microtask to execute because the initial result would always be null until the fill method got called.

Because real code is not as simple as above and some of the logic was buried under different abstractions, it took us a while to realize that an extra pump() was needed in order to get the actual widget rendered.

There are many cases like this where the widget author might not even be aware of what's going in the model layer and how many pumps are needed.

One idea that might help is to provide an interactive pump() button. The pump button would be similar to "Step" button in a debugger except it would just flush microtasks and schedule a frame.

devoncarew commented 4 years ago

@mehmetf - to paraphrase your proposal:

So, they would need a process of trial and error to know how many times to pump, and having a UI button for it would make this easier? I wonder if also having something like 'pump until an expression is true' functionality would help, and the UI could report how many pumps were required.

cc @DanTup as this UI seems like it would also lend itself well to VS Code codelens actions

mehmetf commented 4 years ago

they could then manually inspect whether their test would then pass by resuming execution

Right, for instance, it would be great to see how variables in watch list change as pump flushes the microtasks in fake async queue.

pump until an expression is true

That's a good idea. Especially if you combine it with a expect expression.

...
await tester.pumpWidget(myWidget);
expect(text('foo'), findsOneWidget);  
...

Developer pauses at the expect line and clicks pumpUntil which gives you how many frames it would take for that expect expression to evaluate to true.

DanTup commented 4 years ago

cc @DanTup as this UI seems like it would also lend itself well to VS Code codelens actions

CodeLens has to be attached to a location in the editor and shifts the text around, so it might be a bit jarring if we remove/reinsert this each time the debugger breaks. The debug toolbar might be a better place (assuming we don't hit https://github.com/microsoft/vscode/issues/69398 might prevent this).

Is there a way we can pump the queue from the debugger (while we're paused at a breakpoint without resuming execution)?

mehmetf commented 4 years ago

Is there a way we can pump the queue from the debugger (while we're paused at a breakpoint without resuming execution)?

Do we have the ability to do expression evaluation while paused? A pump IDE action would be equivalent to evaluating tester.pump().

DanTup commented 4 years ago

We can evaluate it, but my understanding was that any async code would not be run while you're paused at breakpoint (this seems like the same scenario as https://github.com/dart-lang/sdk/issues/40198).

DanTup commented 4 years ago

I tried to test this by just evaluating tester.pump() while at a breakpoint, but when I resume, it seems to fail the check and then also throw trying to parse a stack trace:

Screenshot 2020-07-08 at 14 30 21
mehmetf commented 4 years ago

That creates an interesting dilemma because there's no true async code in widget tests. They run in fake async environment provided by package:fake_async. The only thing pump does is flush microtask queue (synchronously)

I guess we should test it out.

DanTup commented 4 years ago

Ah, interesting - if it's completely synchronous I would expect it to work. It's possible the exception is something else (I couldn't see the original because of the exception parsing the stack trace), I just assumed as it's in the same place as without the pump it would be that. I'll see if I can fix get at the original exception to see what's going on.

DanTup commented 4 years ago

Ok, with a little fiddling I got the original exception (it has a bad location, which is why DiagnosticsStackTrace failed to parse it):

Screenshot 2020-07-08 at 15 18 11

It seems like expect is checking (via TestAsyncUtils.guardSync();) that the pump was called from nested inside the test body. So even if the pump works, it seems like it breaks other things if it hadn't been called from within the method directly. (I'm not sure whether it's possible for us to do this when evaluating - right now this was using evaluateInFrame).

Edit: Re-reading the message, it might also be that the pump call hasn't completed. I noticed internally there's a .then() chained into the pump function so it's possible that isn't being completed because we're paused at a breakpoint.

mehmetf commented 4 years ago

That's unfortunate. We need to discuss this with the owners of flutter_test (@chunhtai @goderbauer) to see if there's a good solution.

chunhtai commented 4 years ago

If I read the code correctly, the tester.pump flush the microtask synchronously but create a async future in a sub async zone. https://github.com/flutter/flutter/blob/a82005a9dc71f72fbdbf412a97f911b60f53b648/packages/flutter_test/lib/src/test_async_utils.dart#L70 so you should have to await the future. The error message is checking whether every async future created through TestAsyncUtils.guardSync has bee resolved

DanTup commented 4 years ago

so you should have to await the future

I don't think there's any way for us to await a future being created in a debugger expression evaluation - I think this is similar to https://github.com/dart-lang/sdk/issues/40198. Because the isolate is paused, the future doesn't really completes until we resume (because nothing is pumping the queue), and there's no way for the resume to wait for the Future created in the evaluation expression.

mehmetf commented 4 years ago

Solving this bug requires an IDE specific version of pump, then. The guards are put in place because we want to make sure the test authors are using the APIs correctly. However, IDE does not have that problem so it might make sense to extract this functionality.

Can debugger evaluate a private method? For instance, can you do something like tester.binding._pumpInternal() ? If that's possible then we can take this code out of pump:

      if (duration != null)
        _currentFakeAsync.elapse(duration);
      _phase = newPhase;
      if (hasScheduledFrame) {
        addTime(const Duration(milliseconds: 500));
        _currentFakeAsync.flushMicrotasks();
        handleBeginFrame(Duration(
          milliseconds: _clock.now().millisecondsSinceEpoch,
        ));
        _currentFakeAsync.flushMicrotasks();
        handleDrawFrame();
      }
      _currentFakeAsync.flushMicrotasks();

and put it in a private function.

DanTup commented 4 years ago

Can debugger evaluate a private method?

Doesn't appear so in my testing (Class 'WidgetTester' has no instance method '_foo') but I don't know if there's around that.

If that's possible then we can take this code out of pump:

The crash doesn't happen in pump, it actually appears in expect afterwards. I removed the guard from expect and then called pump() in the debugger and resumed and it did work though, my test case passes (which required a pump but didn't have one in the test code directly).

mehmetf commented 4 years ago

The crash doesn't happen in pump, it actually appears in expect afterwards.

That's normal.

The following code will crash at expect even though the fix is to await the pump.

testWidgets('test', (tester) {
  test.pump();
  expect(find('text'), findsOneWidget);
});

If you remove TestAsyncUtils.guard calls from here:

https://github.com/flutter/flutter/blob/a1a5a8f6357788a3f5e1c4e60a4ae9abf9e6aed2/packages/flutter_test/lib/src/widget_tester.dart#L566

and here:

https://github.com/flutter/flutter/blob/a1a5a8f6357788a3f5e1c4e60a4ae9abf9e6aed2/packages/flutter_test/lib/src/binding.dart#L932

the test will pass. That's what I meant by extracting the pump() body (https://github.com/flutter/flutter/blob/a1a5a8f6357788a3f5e1c4e60a4ae9abf9e6aed2/packages/flutter_test/lib/src/binding.dart#L935-L947) to a private function and calling it directly in IDE rather than calling tester.pump() which is guarded. I was really hoping that debugger is able to evaluate and see private expressions. Perhaps that's an FR?

DanTup commented 4 years ago

to a private function and calling it directly in IDE rather than calling tester.pump() which is guarded

When testing, I removed the guards in the real pump but it still failed in expect. I had to also remove the guard call inside expect to prevent the crash (though I won't pretend to understand exactly why 😬).

I was really hoping that debugger is able to evaluate and see private expressions. Perhaps that's an FR?

Would it be better exposed as a Flutter VM service extension? That's how some of the other debug functionality is called (for ex. toggling debug painting, changing animation speeds etc.). Having an IDE call service extensions feels a bit more natural than evaluating some specific expression (it also avoids any complications around scope or if the user has used a different name than tester). I'm not entirely familiar with the implementations of service extensions though, so it may be good to have someone elses opinion on that (perhaps @devoncarew).

DanTup commented 4 years ago

I had to also remove the guard call inside expect to prevent the crash

Scratch that - I re-tested and it was fine. I think perhaps I didn't remove the guards from both of the places above :)

Giuspepe commented 3 years ago

Hello all, is there any update on this? This feature would help a lot when writing tests. Thanks!