-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: add unit tests for src/timeout.ts #9013
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
base: master
Are you sure you want to change the base?
Conversation
Adds unit tests for the `timeoutFallback` and `timeoutError` functions in `src/timeout.ts`. The tests cover the following scenarios: - Promise resolves before timeout - Timeout occurs before promise resolves - Custom error messages and objects for `timeoutError`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @joehan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces comprehensive unit tests for the timeoutFallback
and timeoutError
utility functions located in src/timeout.ts
. The new tests ensure the robustness and correctness of these functions by covering various asynchronous scenarios, including successful promise resolution and different timeout conditions with appropriate error handling or fallback mechanisms.
Highlights
- New Test File: A new test file, src/timeout.spec.ts, has been added to the project.
- timeoutFallback Function Tests: Unit tests for the
timeoutFallback
function have been implemented, verifying its behavior when a promise resolves successfully before the timeout and when the fallback value is returned due to a timeout. - timeoutError Function Tests: Unit tests for the
timeoutError
function have been implemented, covering scenarios where a promise resolves before timeout, and when a timeout occurs, including cases with default error messages, custom error messages, and custom error objects.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
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.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds a good set of unit tests for the timeout utility functions. The test cases cover the main scenarios correctly. I've added a couple of suggestions to improve the robustness and reliability of the tests. One suggestion is about using fake timers to avoid flakiness, and the other is about improving type safety in error handling within the tests. Overall, great work on adding test coverage.
describe("timeoutFallback", () => { | ||
it("should resolve with the promise value when it completes before timeout", async () => { | ||
const promise = new Promise<string>((resolve) => setTimeout(() => resolve("success"), 10)); | ||
const result = await timeoutFallback(promise, "fallback", 20); | ||
expect(result).to.equal("success"); | ||
}); | ||
|
||
it("should resolve with the fallback value when timeout occurs", async () => { | ||
const promise = new Promise<string>((resolve) => setTimeout(() => resolve("success"), 30)); | ||
const result = await timeoutFallback(promise, "fallback", 20); | ||
expect(result).to.equal("fallback"); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests rely on setTimeout
with real timers, which can sometimes lead to flaky tests on slower environments or under heavy load. Consider using fake timers (e.g., from sinon.js
or Jest's built-in fake timers if you were using Jest) to make these tests deterministic and faster. With fake timers, you can control the passage of time manually (e.g., clock.tick(30)
) without actually waiting.
} catch (e: any) { | ||
expect(e.message).to.equal("Operation timed out."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While using e: any
in a catch block works, it's generally better for type safety to catch the error as unknown
(the default in strict mode) and then perform type checks. This makes the test more robust.
This same improvement can be applied to the other rejection tests in this file (lines 41-43 and 52-54).
} catch (e) {
expect(e).to.be.an.instanceof(Error);
expect((e as Error).message).to.equal("Operation timed out.");
}
Description
From Jules:
Adds unit tests for the
timeoutFallback
andtimeoutError
functions insrc/timeout.ts
.The tests cover the following scenarios:
timeoutError