chore(firestore): fix firestore flaky tests#17439
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors several Firestore system tests to replace fragile sleep delays and polling loops with event-driven synchronization using concurrent.futures.Future. The review feedback highlights two issues: in test_watch_query_order, the future is resolved prematurely on the first snapshot callback instead of waiting for all 5 documents, and in test_create_document_w_vector, accumulating results with += in on_snapshot can lead to duplicate documents and should be replaced by direct assignment.
| if not five_docs_future.done(): | ||
| five_docs_future.set_result(True) |
There was a problem hiding this comment.
The future is currently resolved on the very first snapshot callback, regardless of how many documents are returned. Since the test writes 5 documents and expects to verify their ordering, resolving the future prematurely when len(docs) < 5 will cause the test to pass without actually verifying the ordering of all 5 documents. We should only resolve the future when len(docs) == 5.
| if not five_docs_future.done(): | |
| five_docs_future.set_result(True) | |
| if len(docs) == 5 and not five_docs_future.done(): | |
| five_docs_future.set_result(True) |
| def on_snapshot(docs, changes, read_time): | ||
| on_snapshot.results += docs | ||
| if len(on_snapshot.results) >= 3 and not vector_query_future.done(): | ||
| vector_query_future.set_result(True) |
There was a problem hiding this comment.
Using += to accumulate results in on_snapshot can lead to duplicate documents if multiple snapshots are delivered. Instead, assign the latest docs list directly to on_snapshot.results and check its length to resolve the future safely.
| def on_snapshot(docs, changes, read_time): | |
| on_snapshot.results += docs | |
| if len(on_snapshot.results) >= 3 and not vector_query_future.done(): | |
| vector_query_future.set_result(True) | |
| def on_snapshot(docs, changes, read_time): | |
| on_snapshot.results = docs | |
| if len(docs) >= 3 and not vector_query_future.done(): | |
| vector_query_future.set_result(True) |
Fixes: #17428
Additional change: using 10 workers instead of
autoseems to be a sweet spot.