-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
base: master
Are you sure you want to change the base?
Conversation
…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>({ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- Breaking change
- fakeAsync require sync execution flow, which lead us to critical differences as:
- runZonedGuarded sync execution
- different error/act/etc handling
- possible confusion / false positive results of usage same method for fakeAsync / normal execution - https://github.com/dart-lang/fake_async/issues/38
- and most important fakeAsync issues, which prevent us to mix fakeAsync / normal execution in the same Zone:
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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]. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this 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
There was a problem hiding this 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? 🙌
…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
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. :) |
This PR would be really useful for us, thumbs up 👍 |
Also would be happy to see this |
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. |
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
Comparison
bloc_bloc_test_test.dart average execution time:
bloc_fake_async_bloc_test_test.dart - same test cases, but with testBlocFakeAsync:
Type of Change