Retry polling event queue always, with backoff #515
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This now has some conflicts; would you mind resolving those? |
|
Sure — done. |
chrisbobbe
approved these changes
Feb 22, 2024
Comment on lines
+71
to
-73
| lastRequest = request; | ||
| final response = _nextResponse!; | ||
| _nextResponse = null; | ||
| lastRequest = request; |
There was a problem hiding this comment.
fake_api: Record premature requests, too, in lastRequest
Ah I think I see: a request is "premature" because it's made before its response has been prepared. So the difference is that in those cases (the cases where _nextResponse! fails), the premature request will be recorded as lastRequest, vs. not recorded.
This calls FakeAsync.flushMicrotasks on the FakeAsync that the tester is using. (In the AutomatedTestWidgetsFlutterBinding case, that is, i.e. the `flutter test` case, which is where the tester is already using a FakeAsync.) Seems like it'd be dubious to try to introduce our own FakeAsync (as with `awaitFakeAsync`) when already in another's zone. It's possible it would all work fine, but it seems complicated to have to think about.
This is pretty much transcribed straight from the JS implementation (and test) in zulip-mobile, in the files: src/utils/async.js src/utils/__tests__/async-test.js
This makes it a bit less close of a transcription from the JS version in zulip-mobile (which is why I didn't squash this into the previous commit), but I think a bit more idiomatic as Dart.
This seems like the least confusing behavior.
This has been a useful pattern for other test fakes and mocks.
All the tests still behave the same after this change, because any subsequent checks on `lastRequest` in each test look for a different value.
The new TODO comments are all things that we don't do today in zulip-mobile, and haven't been aware of causing problems for people. So there's no rush to do them here. Fixes: zulip#184
|
Merged. Thanks for the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #184
Along the way, add some infrastructure to help us better write tests for asynchronous code, and in particular to better use
package:fake_async.