felangel / bloc

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

feat: Test support for BlocObservers #4203

Open Rodsevich opened 2 months ago

Rodsevich commented 2 months ago

Description

ATM BlocObservers are being overriden in blocTest logic, and I'm not sure they work as expected when errors are thrown in them (for sure they aren't being outputted on the errors: matcher)

Desired Solution

Add another parameter to blocTest with the oberserver they should use, fully functional ones.

Alternatives Considered

I found workarounds (like defining them on build() everytime and using runZoneGuarded in tests), but I think this is a nice to have, and not that hard to implement neither.

felangel commented 2 months ago

Hi @Rodsevich 👋 Thanks for opening an issue!

BlocObservers should not be overridden in blocTests. Can you please provide a link to a minimal reproduction sample that demonstrates the issue you're facing and I'm happy to take a closer look, thanks!

Rodsevich commented 2 months ago

Sure! This is a workaround working test
And here would be a minimal example of the code changes maybe needed: #4205

felangel commented 2 months ago

@Rodsevich I'm still not sure I understand the problem. Can you please provide more context regarding what the specific issue is and ideally provide a minimal reproduction sample? It's not clear what the problem is based on the code you linked to, thanks!

Rodsevich commented 2 months ago

Sorry, @felangel. I misunderstood your request. I would like to achieve a way of including BlocObserver's behavior in tests, specially when they fail. ATM that's not possible without hard workarounds like the link provided before.
I'd love bloc_test provides the tool for testing that properly, e.g.:

class FailingObserver extends BlocObserver {
  @override
  void onEvent(Bloc<dynamic, dynamic> bloc, Object? event) {
    super.onEvent(bloc, event);
    throw StateError('catch me');
  }
}

blocTest<CounterBloc, int>(
  'should fail on event addition when FailingObserver provided',
  build: () => CounterBloc(),
  act: (bloc) => bloc.add(CounterEvent.increment),
  errors: () => [isA<StateError>()],
  observer: FailingObserver(),
)

I inspired and finished a working proposal in #4205

felangel commented 2 months ago

@Rodsevich thanks for the additional context. I'm not sure modifying blocTest makes sense since blocTest is meant to help you unit test your blocs not your BlocObserver. It should be fairly straightforward to unit test a BlocObserver just like any other plain Dart class. Is there anything specifically challenging about using a regular test to test your BlocObserver?

Rodsevich commented 2 months ago

@felangel thank for your response. The challenge relies on having your BlocObserver solution tested end to end. Take the example I gave you before, how should you test your analytics/logging/etc solution made for BLoCs is working without a test over a BLoC? You wanna ensure your BLoCs will behave as you expect, not your observers. Something similar should happen with Bloc.transformer... How could you be sure your blocs behave the way you want by just testing a stream transformer? Maybe I'm not a testing purist but just a pragmatic. For guys like me an optional parameter that defines the observer or transformer of the BLoCs you are making doesn't bothers.

felangel commented 2 months ago

@felangel thank for your response. The challenge relies on having your BlocObserver solution tested end to end. Take the example I gave you before, how should you test your analytics/logging/etc solution made for BLoCs is working without a test over a BLoC? You wanna ensure your BLoCs will behave as you expect, not your observers. Something similar should happen with Bloc.transformer... How could you be sure your blocs behave the way you want by just testing a stream transformer? Maybe I'm not a testing purist but just a pragmatic. For guys like me an optional parameter that defines the observer or transformer of the BLoCs you are making doesn't bothers.

Since these are unit tests, I'd argue that you should only be testing the BlocObserver or the Bloc (not both). You also don't need to worry about unit testing the integration since that's already covered by the bloc library's test suite. You can assume that when you set the Bloc.observer that works as expected (because it's handled by the library) and same goes for Bloc.transformer.

Hope that helps!

Rodsevich commented 2 months ago

The thing is if we should have behavioral changing objects in the unitTesters or not, right? I think this is turning philosophical 🤔. I love it! Here is an abstraction for arguing the idea: Suppose you wanna test your ducks; so you make a duckTester. The environment in which the duck moves defines the behavior of the duck, for sure, so I add a setter for the environment in the duckTester parameters. Now your ducks can be tested when they are flying/swimming/walking by defining air/lake/ground with ease. How should I do the testing properly? And if the answer is defining unit tests for flying and air objetcs. How should I change the code for getting a tool for integration testing in the bloc_test repo (or which is it if it already exists, for I didn't find it before)?

felangel commented 2 months ago

@Rodsevich BlocObserver shouldn't affect the behavior of blocs though. BlocObserver is a separate component that can observe/react to changes in blocs but generally the behavior of a bloc should not be affected by BlocObserver.

Rodsevich commented 2 months ago

But they can, can't them? How do you recommend implementing something that tweaks the behavior of all your BLoCs? I know, the proper word would be an interceptor or something alike. But why making it more complex if "simplicity is the ultimate sophistication"?

felangel commented 4 weeks ago

But they can, can't them? How do you recommend implementing something that tweaks the behavior of all your BLoCs? I know, the proper word would be an interceptor or something alike. But why making it more complex if "simplicity is the ultimate sophistication"?

Can you provide some more context regarding the use-case your have? I'd love to learn more about how you're using BlocObserver, thanks!

Rodsevich commented 4 weeks ago

Sure! I want a way of defining analytics on BlocEvents. For that I use this observer that checks if they are present on the events and send 'em. Please note the Exceptions thrown... I want everything broken if the setup is faulty or something.

felangel commented 1 week ago

Sure! I want a way of defining analytics on BlocEvents. For that I use this observer that checks if they are present on the events and send 'em. Please note the Exceptions thrown... I want everything broken if the setup is faulty or something.

Wouldn't it be sufficient to have a unit test to make sure that you are overriding the BlocObserver and then separate unit tests testing your custom BlocObserver?

Rodsevich commented 1 week ago

If you want to unit test in general your BlocObserver, yes. But if you want to test your BLoCs, I don't think so... Take the duck example again: I can have a proper Ground object perfectly tested and functional. But when I code I focus on my ducks, and if my business logic of the ducks pretend them to jump on obstacles how can a test that ensures there are obstacles there help me on that purpose?