Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

blocTest does not catch all emitted states #724

Closed
MohsinN opened this issue Dec 13, 2019 · 6 comments
Closed

blocTest does not catch all emitted states #724

MohsinN opened this issue Dec 13, 2019 · 6 comments
Assignees
Labels
pkg:bloc_test This issue is related to the bloc_test package question Further information is requested
Milestone

Comments

@MohsinN
Copy link

MohsinN commented Dec 13, 2019

bloc_test does not catch states emitted by consecutive events added inside bloc. Same scenario passes in plain test but fails with blocTest.

I updated your flutter_login sample to replicate the issue.

Updated AuthenticationBloc to add LoggedOut event inside case of LoggedIn

import 'dart:async';

import 'package:bloc/bloc.dart';
import 'package:flutter_login/authentication/authentication.dart';
import 'package:meta/meta.dart';
import 'package:user_repository/user_repository.dart';

class AuthenticationBloc
    extends Bloc<AuthenticationEvent, AuthenticationState> {
  final UserRepository userRepository;

  AuthenticationBloc({@required this.userRepository})
      : assert(userRepository != null);

  @override
  AuthenticationState get initialState => AuthenticationUninitialized();

  @override
  Stream<AuthenticationState> mapEventToState(
    AuthenticationEvent event,
  ) async* {
    if (event is AppStarted) {
      final bool hasToken = await userRepository.hasToken();

      if (hasToken) {
        yield AuthenticationAuthenticated();
      } else {
        yield AuthenticationUnauthenticated();
      }
    }

    if (event is LoggedIn) {
      yield AuthenticationLoading();
      await userRepository.persistToken(event.token);
      yield AuthenticationAuthenticated();
      add(LoggedOut());
    }

    if (event is LoggedOut) {
      yield AuthenticationLoading();
      await userRepository.deleteToken();
      yield AuthenticationUnauthenticated();
    }
  }
}

In the first case, only first three states are caught by blocTest
Updated tests:

blocTest(
        ' Failing test: emits [uninitialized, loading, authenticated, loading, unauthenticated] when token is persisted',
        build: () => authenticationBloc,
        act: (bloc) => bloc.add(LoggedIn(
              token: 'instance.token',
            )),
        expect: [
          AuthenticationUninitialized(),
          AuthenticationLoading(),
          AuthenticationAuthenticated(),
          AuthenticationLoading(),
          AuthenticationUnauthenticated(),
        ]);

    test(
        'emits [uninitialized, loading, authenticated, loading, unauthenticated] when token is persisted',
        () {
      final expectedResponse = [
        AuthenticationUninitialized(),
        AuthenticationLoading(),
        AuthenticationAuthenticated(),
        AuthenticationLoading(),
        AuthenticationUnauthenticated(),
      ];

      expectLater(
        authenticationBloc,
        emitsInOrder(expectedResponse),
      );

      authenticationBloc.add(LoggedIn(
        token: 'instance.token',
      ));
    });
@felangel
Copy link
Owner

Hi @MohsinN 👋
Thanks for opening an issue!

I'll take a look shortly 👍

@felangel felangel self-assigned this Dec 14, 2019
@felangel felangel added investigating Investigating the issue pkg:bloc_test This issue is related to the bloc_test package labels Dec 14, 2019
@felangel felangel added this to the v3.0.0 milestone Dec 18, 2019
@felangel
Copy link
Owner

Hey sorry for the delayed response (just got back from vacation). I took a closer look and this is actually working as expected. blocTest uses emitsExactly under the hood which closes the bloc's sink and stream in order to provide instant feedback about the test (rather than a potential 30 second timeout). In this case, I would not recommend adding another event for the same bloc within mapEventToState as it's unnecessary and redundant.

You can instead refactor the code like:

import 'dart:async';

import 'package:bloc/bloc.dart';
import 'package:flutter_login/authentication/authentication.dart';
import 'package:meta/meta.dart';
import 'package:user_repository/user_repository.dart';

class AuthenticationBloc
    extends Bloc<AuthenticationEvent, AuthenticationState> {
  final UserRepository userRepository;

  AuthenticationBloc({@required this.userRepository})
      : assert(userRepository != null);

  @override
  AuthenticationState get initialState => AuthenticationUninitialized();

  @override
  Stream<AuthenticationState> mapEventToState(
    AuthenticationEvent event,
  ) async* {
    if (event is AppStarted) {
      final bool hasToken = await userRepository.hasToken();

      if (hasToken) {
        yield AuthenticationAuthenticated();
      } else {
        yield AuthenticationUnauthenticated();
      }
    }

    if (event is LoggedIn) {
      yield AuthenticationLoading();
      await userRepository.persistToken(event.token);
      yield AuthenticationAuthenticated();

      // do logout logic directly or extract into a private helper function
      await userRepository.deleteToken();
      yield AuthenticationUnauthenticated();
    }

    if (event is LoggedOut) {
      yield AuthenticationLoading();
      await userRepository.deleteToken();
      yield AuthenticationUnauthenticated();
    }
  }
}

Hope that helps 👍

@felangel felangel added question Further information is requested and removed investigating Investigating the issue labels Dec 24, 2019
@MohsinN
Copy link
Author

MohsinN commented Dec 28, 2019

Hi,

Thank you for the detailed response.

Great work with library. Also, please let me know if I can contribute in any way. I can help out in writing tests if you like. Open to other suggestions as well.

Do let me know what you think.

@felangel
Copy link
Owner

No problem! If you find anything you feel is lacking please don't hesitate to open an issue and create a pull request 👍

Thanks so much! 🙏

@MohsinN
Copy link
Author

MohsinN commented Dec 30, 2019

Awesome thanks! 😄

dustin-graham added a commit to dustin-graham/bloc that referenced this issue Jan 2, 2020
The issue was that emitsExactly was listening to events too late. Since
Bloc uses a BehaviorSubject, if there is a delay between events the
listener would miss states emitted before the listener attaches. The
listener should start listening before any events are added so that all
states get recorded. I suspect this was the underlying problem with
[felangel#724].
@felangel felangel mentioned this issue Jan 2, 2020
3 tasks
@algodave
Copy link

algodave commented Jan 6, 2020

@felangel From your comment above:

I would not recommend adding another event for the same bloc within mapEventToState

Is it a general rule? In other words, is it an anti-pattern to invoke add() in mapEventToState()? And, what if add() is invoked on another BLoC object instead?

Thanks in advance for your advice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:bloc_test This issue is related to the bloc_test package question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants