felangel / bloc

A predictable state management library that helps implement the BLoC design pattern
https://bloclibrary.dev
MIT License
11.77k stars 3.39k forks source link

test: Calling compute (so spawning new isolates) breaks tests #3097

Open alberto-sf opened 2 years ago

alberto-sf commented 2 years ago

Description

It looks like the compute method breaks the blocTest utility when executed. Consider this code:

import 'dart:io';

import 'package:bloc_test/bloc_test.dart';
import 'package:flutter/foundation.dart';
import 'package:flutter_bloc/flutter_bloc.dart';
import 'package:flutter_test/flutter_test.dart';

int intenseTask(String value) {
  int len = 0;

  for (var i = 0; i < 99999999; ++i) {
    len = i;
  }

  sleep(const Duration(seconds: 1));

  return value.length + len;
}

class Demo extends Bloc<String, int> {
  Demo() : super(0) {
    on<String>(_onString);
  }

  Future<void> _onString(String value, Emitter<int> emit) async {
    if (value.isEmpty) {
      emit(0);
    } else {
      final result = await compute<String, int>(intenseTask, value);

      emit(result);
    }
  }
}

void main() {
  group('Demo test', () {
    blocTest<Demo, int>(
      'Works',
      build: () => Demo(),
      act: (bloc) => bloc.add(''),
      expect: () => const [0],
    );

    blocTest<Demo, int>(
      "Doesn't work",
      build: () => Demo(),
      act: (bloc) => bloc.add('using the compute method'),
      verify: (bloc) => expect(bloc.state, greaterThan(0)),
    );
  });
}

The 'Works' test runs fine since we aren't calling compute but 'Doesn't work' fails with this error:

Expected: a value greater than <0> Actual: <0> Which: is not a value greater than <0>

Error resuming isolate isolates/2923108837404163: -32000 Bad state: The client closed with pending request "resume".

Am I missing something to make this work? Or is this a bug related to isolates and the bloc's running zone?

albertodev01 commented 2 years ago

Felix I submitted this with my other work account. I think you'd need to change a bit the blocTest definition:

void blocTest<B extends BlocBase<State>, State>(
  String description, {
  FutureOr<void> Function()? setUp,
  required B Function() build,
  State Function()? seed,
  Function(B bloc)? act,
  Duration? wait,
  int skip = 0,
  dynamic Function()? expect,
  Function(B bloc)? verify,
  dynamic Function()? errors,
  FutureOr<void> Function()? tearDown,
  dynamic tags,
  bool runAsync = false, // add this
}) {
  if (runAsync) {
     await tester.runAsync(() {
       test.test( /* current implementation you have */ );
     });
  } else {
    test.test( /* current implementation you have */ );
  }  
}

Not sure if bool runAsync is good or if you should setup a callback to pass in a WidgetTester but you need something to tell blocTest that the test is going to spawn a new Isolate.

I didn't test this but the idea is that when you have compute (or in general when you spawn new isolates with different Zone.root), you should use runAsync. See the docs, but the relevant line is:

This is intended for callers who need to call asynchronous methods where the methods spawn isolates or OS threads and thus cannot be executed synchronously by calling pump.

It's also true that I could call it by myself like

await tester.runAsync() {
  blocTest<Demo, int>(
    "Doesn't work",
    build: () => Demo(),
    act: (bloc) => bloc.add('using the compute method'),
    verify: (bloc) => expect(bloc.state, greaterThan(0)),
  );
}

but I think it's error-prone for people that don't know how zones and isolates work. If this was already in bloc_test it would be great 😄 what do you think?

felangel commented 2 years ago

Hi @albertodev01 👋 I took a closer look and I believe the issue is the blocTest is ending early before the compute has finished. See the updated test below:

import 'dart:io';

import 'package:bloc_test/bloc_test.dart';
import 'package:flutter/foundation.dart';
import 'package:bloc/bloc.dart';
import 'package:test/test.dart';

int intenseTask(String value) {
  int len = 0;

  for (var i = 0; i < 10; ++i) {
    len = i;
  }

  sleep(const Duration(seconds: 1));

  return value.length + len;
}

class Demo extends Bloc<String, int> {
  Demo() : super(0) {
    on<String>(_onString);
  }

  Future<void> _onString(String value, Emitter<int> emit) async {
    if (value.isEmpty) {
      emit(0);
    } else {
      final result = await compute<String, int>(intenseTask, value);

      emit(result);
    }
  }
}

void main() {
  group('Demo test', () {
    blocTest<Demo, int>(
      'Works',
      build: () => Demo(),
      act: (bloc) => bloc.add(''),
      expect: () => const [0],
    );

    blocTest<Demo, int>(
      "Also works",
      build: () => Demo(),
      act: (bloc) => bloc.add('using the compute method'),
      wait: Duration(seconds: 10),
      verify: (bloc) => expect(bloc.state, greaterThan(0)),
    );
  });
}

This works as expected. Let me know what you think, thanks!

albertodev01 commented 2 years ago

Thank you for your time 😄

I think this is more of a work-around rather than a solution since if you set it to be like Duration(milliseconds: 1) it wouldn't work. What works is this instead:

await tester.runAsync() { ... }

If you read the runAsync docs you'll notice that it exactly server for this purpose!

This is intended for callers who need to call asynchronous methods where the methods spawn isolates or OS threads and thus cannot be executed synchronously by calling pump.

It basically waits for other isolated forked from the root to finish. Do you think you could add a runAsync call inside bloc test like this?

void blocTest<B extends BlocBase<State>, State>(  
  bool runAsync = false, // add this
}) {
  if (runAsync) {
     await tester.runAsync(() {
       test.test( /* current implementation you have */ );
     });
  } else {
    test.test( /* current implementation you have */ );
  }  
}

Otherwise we could simply wrap blocTest with runAsync but in that case I think you should document it 🙂

felangel commented 2 years ago

Hi @albertodev01

I don't think this is a workaround. The test is failing because it isn't waiting for the async operation to complete. Using tester.runAsync won't change that afaik. Also, tester.runAsync is from Flutter and requires flutter_test and a WidgetTester whereas bloc_test should be a pure dart package and should not be used in widget tests.

Do you have a link to a sample illustrating the same test failing without tester.runAsync and passing with it? Thanks!

felangel commented 2 years ago

@albertodev01 just wanted to see if you had a chance to see my reply, thanks!

albertodev01 commented 2 years ago

@felangel Sorry I forgot to reply!

Also, tester.runAsync is from Flutter and requires flutter_test and a WidgetTester whereas bloc_test should be a pure dart package and should not be used in widget tests.

Ahh I get your point 😄 I think that the best thing to do then is adding a docstring about the tester.runAsync() method when using Flutter.

In order to keep this a "pure" Dart library, users will need to do as you did (using wait: Duration(seconds: 10), with a random time value or anyway bigger than the computational time required by compute)

felangel commented 2 years ago

@felangel Sorry I forgot to reply!

Also, tester.runAsync is from Flutter and requires flutter_test and a WidgetTester whereas bloc_test should be a pure dart package and should not be used in widget tests.

Ahh I get your point 😄 I think that the best thing to do then is adding a docstring about the tester.runAsync() method when using Flutter.

In order to keep this a "pure" Dart library, users will need to do as you did (using wait: Duration(seconds: 10), with a random time value or anyway bigger than the computational time required by compute)

No problem!

Ideally, we'd mock the long-running operation so that the test runs efficiently/quickly.

Wolfteam commented 2 years ago

Hi, I'm migrating from bloc_test: ^8.5.0 to bloc_test: ^9.0.2 and I think I've the same problem. Was the behaviour of blocTest changed on the latest version ? I've a test that passes on the 8.5.0 version but fails on the latest one, though, if I add the wait parameter it works

felangel commented 2 years ago

Hi, I'm migrating from bloc_test: ^8.5.0 to bloc_test: ^9.0.2 and I think I've the same problem. Was the behaviour of blocTest changed on the latest version ? I've a test that passes on the 8.5.0 version but fails on the latest one, though, if I add the wait parameter it works

Can you share a minimal reproduction sample of the code that works in 8.5.0 and fails in 9.0.0? Thanks!

Wolfteam commented 2 years ago

Hi, I'm migrating from bloc_test: ^8.5.0 to bloc_test: ^9.0.2 and I think I've the same problem. Was the behaviour of blocTest changed on the latest version ? I've a test that passes on the 8.5.0 version but fails on the latest one, though, if I add the wait parameter it works

Can you share a minimal reproduction sample of the code that works in 8.5.0 and fails in 9.0.0? Thanks!

Here's a minimal sample:

testing.zip

To test the old version, change the following files:

felangel commented 2 years ago

Hi, I'm migrating from bloc_test: ^8.5.0 to bloc_test: ^9.0.2 and I think I've the same problem. Was the behaviour of blocTest changed on the latest version ? I've a test that passes on the 8.5.0 version but fails on the latest one, though, if I add the wait parameter it works

Can you share a minimal reproduction sample of the code that works in 8.5.0 and fails in 9.0.0? Thanks!

Here's a minimal sample:

testing.zip

To test the old version, change the following files:

  • In the pubspec.yaml file you will find commented the old versions used.
  • In the some_bloc.dart uncomment the part that says OLD VERSION and comment the NEW VERSION
  • If you run flutter test the only test there should pass, but if you try with the newer version it should fail unless you set the wait parameter on the test

Are you able to push the code to a GitHub repo or Gist rather than sharing a .zip?

Wolfteam commented 2 years ago

Hi, I'm migrating from bloc_test: ^8.5.0 to bloc_test: ^9.0.2 and I think I've the same problem. Was the behaviour of blocTest changed on the latest version ? I've a test that passes on the 8.5.0 version but fails on the latest one, though, if I add the wait parameter it works

Can you share a minimal reproduction sample of the code that works in 8.5.0 and fails in 9.0.0? Thanks!

Here's a minimal sample: testing.zip To test the old version, change the following files:

  • In the pubspec.yaml file you will find commented the old versions used.
  • In the some_bloc.dart uncomment the part that says OLD VERSION and comment the NEW VERSION
  • If you run flutter test the only test there should pass, but if you try with the newer version it should fail unless you set the wait parameter on the test

Are you able to push the code to a GitHub repo or Gist rather than sharing a .zip?

Done repo