fix(bloc_test)!: forward _TestBlocObserver events to localObserver

[alestiago]

Status

READY

Breaking Changes

MAYBE YES

If the user was defining a Bloc.observer, now it will actually observe. Whereas before it would've been ignored.

Description

Before, only the localObserver will get notified of onError all other obvervables would be silenced.

Type of Change

• ✨ New feature (non-breaking change which adds functionality)
• 🛠️ Bug fix (non-breaking change which fixes an issue)
• ❌ Breaking change (fix or feature that would cause existing functionality to change)
• 🧹 Code refactor
• ✅ Build configuration change
• 📝 Documentation
• 🗑️ Chore

[alestiago]

@felangel friendly ping requesting for a review

[felangel]

@alestiago thanks for the PR and sorry for the delay! I took a look and I'm just wondering if you can provide some context for why this change is necessary. blocTest should only be used for unit testing a single bloc so it should ideally not involve any BlocObservers. Testing a BlocObserver should be done in a separate test and shouldn't require blocTest. Let me know what you think and thanks again!

[alestiago]

There are multiple scenarios where this change would help.

Scenario 1

In some select cases it may make sense for events to be added internally.
Angelov, 2022 (#3633, https://bloclibrary.dev/faqs/#adding-events-within-a-bloc)

One can verify an internally added event (event added within the Bloc) gets added by another externally added event (event added outside the Bloc) easily using a BlocObserver.

Here's a current working approach you could use:

Example 1: Verify EventC is added when handling EventA (Currently working approach)

import 'package:bloc/bloc.dart';
import 'package:bloc_test/bloc_test.dart';
import 'package:example/foo_bloc.dart';
import 'package:test/test.dart';

class _FooBlocObserver extends BlocObserver {
  List<FooEvent> events = [];

  @override
  void onEvent(covariant FooBloc bloc, Object? event) {
    super.onEvent(bloc, event);
    if (event case FooEvent event) events.add(event);
  }
}

void main() {
  blocTest<FooBloc, FooState>(
    'EventA adds EventC',
    seed: () => FooState.something(),
    setUp: () => Bloc.observer = _FooBlocObserver(),
    act: (bloc) => bloc.add(EventA()),
    verify: (bloc) {
      final observer = Bloc.observer as _FooBlocObserver;
      expect(observer.events, containsOnce(isA<EventC>())); // Pass
    },
  );
}

However you can't do (you would be able to if #4688 is merged):

Example 2: Non-working verification due to _TestBlocObserver forwarding

import 'package:bloc/bloc.dart';
import 'package:bloc_test/bloc_test.dart';
import 'package:example/foo_bloc.dart';
import 'package:test/test.dart';

class _FooBlocObserver extends BlocObserver {
  List<FooEvent> events = [];

  @override
  void onEvent(covariant FooBloc bloc, Object? event) {
    super.onEvent(bloc, event);
    if (event case FooEvent event) events.add(event);
  }
}

void main() {
  late _FooBlocObserver observer;

  setUp(() {
    final previousObserver = Bloc.observer;
    addTearDown(() => Bloc.observer = previousObserver);
    observer = _FooBlocObserver();
    Bloc.observer = observer;
  });

  blocTest<FooBloc, FooState>(
    'EventA adds EventC',
    build: () => FooBloc(),
    seed: () => FooState.something(),
    act: (bloc) => bloc.add(EventA()),
    verify: (bloc) {
      // _TestBlocObserver with _localObserver of type _FooBlocObserver.
      Bloc.observer;

      // _TestBlocObserver does not forward onEvent to _localObserver, only
      // forwards onError.
      expect(observer.events, containsOnce(isA<EventC>())); // Fails
    },
  );
}

This also means that in Example 1, although it works, completely overrides _TestBlocObserver meaning that we miss out on the nice collection and printing of errors that _TestBlocObserver does on:

bloc/packages/bloc_test/lib/src/bloc_test.dart

Line 193 in 4d22f5e

unhandledErrors.add,

Notice how blocTest is convenient over test since we can easily seed the Bloc without too much hassle.

Scenario 2

As you can imagine using a similar approach as the one outlined in Example 1 could help us test that a given event chain doesn't take more than a specified amount of time to complete. For example, by observing onDone and onEvent and an internal Stopwatch that gets triggered and stopped when it sees the tracked events.

You could also check if X doesn't happen before EventA completes and so on.

Scenario 3

Consitency. For some reason only onError is forwarded by _TestBlocObserver all other observable methods are ignored. #4688 adds the consistency.

---

Conclusion

In conclusion, in most cases checking the emitted states of an event is enough, however in some select cases it doesn't. In all, using observability gives more testing power for verifying desired intricate behaviour from a user-defined Bloc. Using observability is already supported and allowed in tests but it would be more convenient in terms of written code and error reporting to follow the pattern in Example 2 over the one in Example 1 (so _TestBlocObserver is not completely overrided).

Let me know your thoughts or if you have other approaches.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[felangel]

Thanks for the context!

Regarding testing internal events, I would focus on testing the output of those events rather than trying to verify that the internal event was added. In the example you shared, the EventC isn't an internal event because it's part of the public API. In most cases, internal events should be private and consequently inaccessible from a unit test.

The event timing scenario you describe is interesting but that also seems like not something that should be verified in a unit test. You'd likely want separate performance/profiling tests which don't mock dependencies for that type of test.

I'm fine with merging the changes from a consistency standpoint but I would ideally also like the have a good use case to back the changes (especially because they are technically breaking changes). I'll think about it a bit more... 🤔

[alestiago]

Thanks for the reply!

I've updated the terminology used in the previous comment to internally added event and externally added event to avoid confusion with the internal word used in the context of API visibility. So, now in the example EventC is both an internally added event and an externally added event.

Regarding testing internal events, I would focus on testing the output of those events rather than trying to verify that the internal event was added.

I agree most cases should do so! However, I think there's sometimes value on just verifying a call to add was made. Specially in the cases where an internally added event is added by multiple event handlers (for example, EventZ is internally added by EventD, EventE, EventF, EventG, EventH, ... handlers. Maybe because you don't want to re-test all the new states across all handlers, or re-test all the side-effects of EventZ event (not just state outputs) on each event handler that adds EventZ, or because you don't want to have to set-up all the dependencies needed to test EventD to test EventZ, etc -- similarly to how we use mocktail's verify on Dart to avoid re-testing certain calls that have already been properly tested.

I'm fine with merging the changes from a consistency standpoint but I would ideally also like the have a good use case to back the changes (especially because they are technically breaking changes). I'll think about it a bit more... 🤔

Sounds good! Keep me posted 💙

[alestiago]

@felangel any updates?

[felangel]

@felangel any updates?

Sorry for the delay! Will plan to revisit this either today or tomorrow.

[felangel]

LGTM thanks so much and apologies for the delay! 💙