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

feat(bloc_test): testBlocFakeAsync - fire all asynchronous events without actually needing to wait for real time to elapse #3796

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

PankovSerge
Copy link

@PankovSerge PankovSerge commented Apr 18, 2023

Status

READY

Breaking Changes

NO

Description

This pull request aims to address the issue of slow test execution in complex test cases involving multiple events with throttleTime, debounceTime, or similar delays in the current implementation of bloc_test.

To tackle this problem, this pull request introduces a new function called testBlocFakeAsync. This new function is built around FakeAsync which provides a way to simulate and fire all asynchronous events that are scheduled for that time period without actually requiring the test to wait for real time to elapse.

Example

  fakeAsyncBlocTest<DebounceCounterBloc, int>(
        'emits [2] when CounterEvent.increment '
        'is added twice and skip: 1',
        build: () => DebounceCounterBloc(),
        act: (bloc, fakeAsync) async {
          bloc.add(CounterEvent.increment);
          fakeAsync.elapse(const Duration(milliseconds: 305));
          bloc.add(CounterEvent.increment);
        },
        skip: 1,
        wait: const Duration(milliseconds: 300),
        expect: () => const <int>[2],
      );

Comparison

bloc_bloc_test_test.dart average execution time:
Screenshot 2023-04-18 at 23 58 03

bloc_fake_async_bloc_test_test.dart - same test cases, but with testBlocFakeAsync:
Screenshot 2023-04-18 at 23 58 26

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

…hout actually needing to wait for real time to elapse

1. introduced fakeAsyncBlocTest
2. fakeAsyncBlocTest tests -> 100% coverage
3. added fake_async dependency
…hout actually needing to wait for real time to elapse

1. Removed sandbox file
…hout actually needing to wait for real time to elapse

1. Fixed linter warnings -> Avoid lines longer than 80 characters.
/// This should never be used directly -- please use [fakeAsyncBlocTest]
/// instead.
@visibleForTesting
void testBlocFakeAsync<B extends BlocBase<State>, State>({
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method duplicates a lot of code that can be found on testBloc. What about refactoring both to share as much code as possible so maintenance is simpler?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tenhobi @mugbug there's a few reasons to not unify the code:

  1. Breaking change
  2. fakeAsync require sync execution flow, which lead us to critical differences as:

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I get it, but my suggestion was to share internal code, not external. So we'd still have the current blocTest and testBlocFakeAsync methods, but the internal code of them will be somehow shared. In this case we won't have breaking changes since the public API for blocTest will remain the same. Not sure about the 2nd point but I suppose we can choose which code to share, so we should be good as well. Does it make sense?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got your point, @mugbug The duplicated code has been extracted to a private functions.

cc: @tenhobi

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Now I suppose we just need to wait for an approval on the code solution and then work on some examples and documentation updates. Good job!

@@ -170,6 +171,271 @@ void blocTest<B extends BlocBase<State>, State>(
);
}

/// Creates a new `bloc`-specific test case with the given [description].
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we could also add some sort of example using fakeAsync.elapse which is kind of the main purpose of this new method, wdyt?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might wanna update the README.md docs as well and any other sort of documentation bloc_test might have

…hout actually needing to wait for real time to elapse

1. Modified doc comments & fixed typo's
Copy link
Collaborator

@tenhobi tenhobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this as it can make testing blocs faster. πŸ‘

As mentioned, I would also like to see some unification of testBloc and fakeAsyncBlocTest to make it easy to maintain.

…hout actually needing to wait for real time to elapse

1. Replaced ' with ` in doc comments
…hout actually needing to wait for real time to elapse

1. Extracted duplicated code
Copy link
Collaborator

@tenhobi tenhobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look fine by me, just some formatting nits. All tests pass, seems good.

@felangel can you take a look at it? πŸ™Œ

packages/bloc_test/lib/src/bloc_test.dart Outdated Show resolved Hide resolved
packages/bloc_test/lib/src/bloc_test.dart Outdated Show resolved Hide resolved
packages/bloc_test/lib/src/bloc_test.dart Outdated Show resolved Hide resolved
…hout actually needing to wait for real time to elapse

1. Fixed formatting in bloc_test
…_async_support

# Conflicts:
#	packages/bloc_test/lib/src/bloc_test.dart
…hout actually needing to wait for real time to elapse

1. Sync with master & conflicts resolve
@PankovSerge PankovSerge requested a review from tenhobi May 27, 2023 22:59
@tenhobi
Copy link
Collaborator

tenhobi commented Dec 15, 2023

Hey, @felangel, can you look at this? Seems kinda useful. Thanks!

@felangel
Copy link
Owner

Hey, @felangel, can you look at this? Seems kinda useful. Thanks!

Yup will look this weekend. Sorry for the delay!

@tenhobi
Copy link
Collaborator

tenhobi commented Jan 4, 2024

Hey, @felangel, can you look at this? Seems kinda useful. Thanks!

Yup will look this weekend. Sorry for the delay!

So that didn't work out. :D

Since I got another random test fail due to time issues, I remembered this PR and just wanted to ping you. :)

@karvulf
Copy link

karvulf commented Jan 22, 2024

This PR would be really useful for us, thumbs up πŸ‘

@test0terter0n
Copy link

Also would be happy to see this

@felangel
Copy link
Owner

felangel commented May 4, 2024

I'm catching up on this (sorry for the delay) and am going to spend a bit of time to see if there's a way to unify these APIs in a non-breaking way rather than exposing two almost identical APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Support time manipulation in bloc_test with clock/fakeAsync
6 participants