search

adventures-in / chat_app

A group effort to build a chat app and learn as we go.
18 stars 0 forks source link

Fix for range issue by connection state waiting #167

Closed gaslitbytech closed 4 years ago

gaslitbytech commented 4 years ago

Ideally this would be fixed with unittest though a bit overwhelmed with the amount of work required to decouple the _ConversationListState with firestore to allow testing by mock / fake or inject.

Though after seeing the following snippet I noticed some robustness was missed out of our implementation with regards to connection state. That or this is a feature since introduced.

nickmeinhold commented 4 years ago

Awesome work mate! Thanks for working on this!

So were you able to determine that this change fixes the range issue by manual testing? I think it's a good change either way (we should always have a check for connection state - it was probably me being lazy, nice pick up by the way!), just wondering if we should use the opportunity to add a test that fails before and passes after the fix.

We could (and should) decouple the ConversationsList widget from the Firestore instance - we can just move the Firestore call into the DatabaseService alongside the other Firebase calls (should have already been done really), then in the widget use the Provider to get the DatabaseService. In a widget test we can then wrap the ConversationsList widget in a Provider that provides a mocked DatabaseService (that returns a stream with whatever test data we want the widget to receive).

I'm happy to create a separate issue for this if you'd prefer.

We're kicking goals! :-)

gaslitbytech commented 4 years ago

Yes I was able to manually test it and removed the Range Exception from reoccurring.

I'll add another issue that can target the decoupling + testing.