Skip to content

actions test: Await all responses; cut pumpAndSettle #993

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

Merged
merged 1 commit into from
Oct 12, 2024

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Oct 11, 2024

This ensures that if one of these markNarrowAsRead calls were to change behavior so that it no longer completed within the test (and then possibly threw an error, or failed to ever complete at all), the test would catch that.

Prompted by this thread:
#934 (comment)

@gnprice gnprice added the maintainer review PR ready for review by Zulip maintainers label Oct 11, 2024
@gnprice gnprice requested a review from PIG208 October 11, 2024 01:55
@gnprice gnprice mentioned this pull request Oct 11, 2024
@@ -52,8 +52,9 @@ void main() {
processedCount: 11, updatedCount: 3,
Copy link
Member

Choose a reason for hiding this comment

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

There is one more call-site of pumpAndSettle from prepare that can be cut too.

@PIG208
Copy link
Member

PIG208 commented Oct 11, 2024

Looks good! Just one small comment.

This ensures that if one of these markNarrowAsRead calls were to
change behavior so that it no longer completed within the test
(and then possibly threw an error, or failed to ever complete at all),
the test would catch that.
@gnprice
Copy link
Member Author

gnprice commented Oct 12, 2024

Thanks for the review! Merging with that tweak.

@gnprice gnprice merged commit 4239515 into zulip:main Oct 12, 2024
1 check passed
@gnprice gnprice deleted the pr-test-await branch October 12, 2024 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer review PR ready for review by Zulip maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants