Remove test flakiness #181
Conversation
Abestanis
force-pushed
the
feature/test_cleanup
branch
from
c67e075 to
fa7b741
Compare
Abestanis
force-pushed
the
feature/test_cleanup
branch
from
aa6c5f1 to
334cac1
Compare
test/golden/goldens/artist_content_route.artist_content_route_songs.dark.png
Outdated
Show resolved
Hide resolved
test/golden/goldens/player_route.player_route.dark.artColor.png
Outdated
Show resolved
Hide resolved
Abestanis
force-pushed
the
feature/test_cleanup
branch
from
2d8d846 to
25134ad
Compare
Abestanis
force-pushed
the
feature/test_cleanup
branch
from
25134ad to
f5936a1
Compare
There was a problem hiding this comment.
Interestengly, a side effect I see on many screenshots is that the shadow from the drawer seems to have gone
It's OK, just something I noticed reviewing golden changes
| AsyncCallback callback, { | ||
| AsyncCallback? goldenCaptureCallback, | ||
| VoidCallback? initialization, | ||
| VoidCallback? postInitialization, |
There was a problem hiding this comment.
Should we align runAppTestWithoutUi with this one and pass this callback as well?
| /// 4. Runs the test from the [callback]. | ||
| /// 5. Optionally, runs [goldenCaptureCallback]. | ||
| /// 6. Stops and disposes the player. | ||
| /// 7. Un-pumps the screen and flushes all micro-tasks and stream events. |
There was a problem hiding this comment.
I would also cross-reference runAppTestWithoutUi here as "See also", discussing their differences and when one should use one above the other one
Same for the comment in runAppTestWithoutUi
| // Wait for ui animations. | ||
| await pumpAndSettle(); | ||
| // Un-pump, in case we have any real animations running, | ||
| // so the pumpAndSettle on the next line doesn't hang on. |
| await pumpWidget(const SizedBox()); | ||
| // Wait for any asynchronous events and stream callbacks to finish. | ||
| await pump(const Duration(seconds: 1)); | ||
| // Wait for ui animations. |
There was a problem hiding this comment.
I imagine this is referring to "system UI" (but don't remember for sure)
Let's put that into the comment tho
Otherwise pumping out the UI above would already stop animations and we wouldn't need pumpAndSettle
| }, | ||
| () => test(tester), | ||
| goldenCaptureCallback: () => tester.screenMatchesGolden( | ||
| Invoker.current!.liveTest.test.name.split(' | theme')[0].replaceAll(' ', '.'), |
There was a problem hiding this comment.
Filename is already being built here https://github.com/nt4f04uNd/sweyer/blob/master/test/test.dart#L259
I don't completely understand the purpose of this code, but it seems like using the description parameter would already would enrich it with the required parameters, maybe except from the group name
This code just seems to be not very scalable
| @@ -10,46 +10,46 @@ void main() { | |||
| final favoriteSong2 = songWith(id: 4, title: 'Song 4', isFavoriteInMediaStore: true); | |||
| final favoriteSong3 = songWith(id: 5, title: 'Song 5', isFavoriteInMediaStore: true); | |||
|
|
|||
| setUp(() async { | |||
| await setUpAppTest(() { | |||
There was a problem hiding this comment.
Are we sure this is a good API change that we not longer can use setUp between tests?
There was a problem hiding this comment.
I'd argue it's not - it reduces the reusability of setup logic
In some cases it might also make that tests utilities are not composable with each other - this is a real case in tests that I had recently, when I made a not very composable API desicion, which put constraints on how tests could be written, so I had to rewrite it at some point after
There was a problem hiding this comment.
From the description of the PR I see that you want them to live inside the same zone as other test logic
It this possible to achieve in some other way?
This is an attempt to reduce the test flakiness. I noticed that
setUpcalls are not executed on the sameFakeAsynczone than the tests, which means that they are more likely to leak into other tests and flushing mikro-tasks in the tests doesn't actually flush the ones scheduled insetUp. Maybe this is enough to fix the flakiness in the tests. It definitely helped to reveal two instances where we forgot to dispose a timer or animation controller.I also removed all
runAsynccalls. As a general rule of thumb, we don't really want the tests to do any real async work, (network requests, file accesses or even worse, waiting for a timeout should all be mocked), so we should never userunAsync.Fixes #162.