Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
Generated by 🚫 Danger |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses several flaky unit tests by correcting race conditions and improving assertion logic. The changes in FIRMessagingPendingTopicsListTest.m replace brittle, timing-dependent expectations with more robust mechanisms, such as counting completed operations or waiting for the entire queue to be processed. This ensures tests are deterministic. The modification in FIRMessagingExtensionHelperTest.m aligns the test's implementation with its descriptive name, improving the clarity and correctness of the test suite. The fixes are well-implemented and significantly enhance the reliability of the tests.
|
I'm not sure how much of a difference this made. If you look at the files tab in the PR, GitHub Actions left comments on the tests that flaked and 3 of the 4 modified tests still flaked on attempts 1 and 2. |
See the flakes in https://github.com/firebase/firebase-ios-sdk/actions/runs/19787052556/job/56694978667 These have been recurring in nightly test runs.
Here's Gemini's analyis:
✦ I've reviewed FIRMessagingPendingTopicsListTest.m, and the flaky tests are all caused by race conditions in the asynchronous operations. The timeouts occur
because the expectations are not being fulfilled consistently. I've identified the issues and will now fix them.
My plan is to:
testBatchSizeReductionAfterSuccessfulTopicUpdate: The test hangs because the subscriptionHandler is called for every topic operation, but theexpectation is only fulfilled for a single topic. I will introduce a counter to ensure all topic updates are processed before the test completes.
testCompletionOfTopicUpdatesInSameThread: This test fails because it relies on an updateHandler that is not always called. I will add a counter andan explicit expectation to ensure that all topic operations complete successfully.
testAddingTopicToCurrentBatchWhileCurrentBatchTopicsInFlight: This test is flaky because it relies on a fixed delay, which is not a reliable way totest asynchronous code. I will restructure the test to ensure that the "straggler" topic is added at the correct time, and I'll add an expectation to
confirm that both topics are processed.
✦ I've analyzed FIRMessagingExtensionHelperTest.m and found testModifyNotificationWithEmptyPayloadData fails because validPayloadExpectation isn't met. The issue
stems from an invalid image URL preventing the handler block from executing. My fix will entail: 1) correcting the test's content.userInfo to truly represent
an "empty" payload, removing the invalid image URL, and 2) adjusting the expectation to verify loadAttachmentForURL is not called, but the handler is called
with the original content. I'll begin by modifying content.userInfo to reflect an empty payload and confirming loadAttachmentForURL is rejected.