Add unit test for uploader.spec.ts #9543
Conversation
Summary of ChangesHello @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 a new unit test file for the Highlights
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. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds unit tests for the Uploader class, which is a great step towards increasing test coverage. The tests cover the initialization and the main file processing logic, including hashing, populating, and uploading. My review includes a few suggestions to improve the test code's quality and maintainability, such as improving typings in the mock queue, removing unused code, and refactoring duplicated logic in the stream mocks. Overall, this is a solid contribution.
| class MockQueue { | ||
| public handler: any; | ||
| private promises: Promise<void>[] = []; | ||
| constructor(options: any) { | ||
| this.handler = options.handler; | ||
| } | ||
| add(item: any) { | ||
| const p = Promise.resolve(this.handler(item)); | ||
| this.promises.push(p); | ||
| } | ||
| process() { } | ||
| async wait() { | ||
| await Promise.all(this.promises); | ||
| return Promise.resolve(); | ||
| } | ||
| close() { } | ||
| stats() { | ||
| return { total: 0, complete: 0, cursor: 0 }; | ||
| } | ||
| } |
There was a problem hiding this comment.
The MockQueue class uses any in its constructor and methods. It could be made more type-safe by using generics, similar to the actual Queue implementation it's mocking. This would improve the robustness and readability of the test setup. After applying this change, you'll also need to update the instantiation of MockQueue to provide the generic type, for example: new MockQueue<string>({ ... }).
| class MockQueue { | |
| public handler: any; | |
| private promises: Promise<void>[] = []; | |
| constructor(options: any) { | |
| this.handler = options.handler; | |
| } | |
| add(item: any) { | |
| const p = Promise.resolve(this.handler(item)); | |
| this.promises.push(p); | |
| } | |
| process() { } | |
| async wait() { | |
| await Promise.all(this.promises); | |
| return Promise.resolve(); | |
| } | |
| close() { } | |
| stats() { | |
| return { total: 0, complete: 0, cursor: 0 }; | |
| } | |
| } | |
| class MockQueue<T> { | |
| public handler: (item: T) => Promise<void>; | |
| private promises: Promise<void>[] = []; | |
| constructor(options: { handler: (item: T) => Promise<void> }) { | |
| this.handler = options.handler; | |
| } | |
| add(item: T) { | |
| const p = Promise.resolve(this.handler(item)); | |
| this.promises.push(p); | |
| } | |
| process() { } | |
| async wait() { | |
| await Promise.all(this.promises); | |
| return Promise.resolve(); | |
| } | |
| close() { } | |
| stats() { | |
| return { total: 0, complete: 0, cursor: 0 }; | |
| } | |
| } |
| (uploader as any).uploadQueue = new MockQueue({ handler: (uploader as any).uploadHandler.bind(uploader) }); | ||
|
|
||
| (fs.statSync as sinon.SinonStub).returns({ mtime: new Date(), size: 100 }); | ||
| const { PassThrough, Readable } = require("stream"); |
There was a problem hiding this comment.
For consistency with other imports and better code organization, please use an import statement at the top of the file instead of require() within the test. You can remove this line and add import { PassThrough, Readable } from "stream"; to the top of the file.
| // Mock stream for file1.txt | ||
| const mockStream1 = new Readable({ | ||
| read() { | ||
| this.push(Buffer.from("hash1")); | ||
| this.push(null); | ||
| }, | ||
| }); | ||
| // Mock stream for file2.txt | ||
| const mockStream2 = new Readable({ | ||
| read() { | ||
| this.push(Buffer.from("hash2")); | ||
| this.push(null); | ||
| }, | ||
| }); | ||
|
|
||
| (zlib.createGzip as sinon.SinonStub).callsFake(() => new PassThrough()); | ||
| (fs.createReadStream as sinon.SinonStub).callsFake((filePath: string) => { | ||
| if (filePath.includes("file1.txt")) { | ||
| return new Readable({ | ||
| read() { | ||
| this.push(Buffer.from("hash1")); | ||
| this.push(null); | ||
| }, | ||
| }); | ||
| } | ||
| if (filePath.includes("file2.txt")) { | ||
| return new Readable({ | ||
| read() { | ||
| this.push(Buffer.from("hash2")); | ||
| this.push(null); | ||
| }, | ||
| }); | ||
| } | ||
| return new PassThrough(); | ||
| }); |
There was a problem hiding this comment.
This section for mocking streams can be simplified. The mockStream1 and mockStream2 variables are declared but never used and can be removed. Additionally, the Readable stream creation logic is duplicated inside fs.createReadStream.callsFake. This can be extracted to a helper function to make the code cleaner and more maintainable.
const createMockStream = (content: string) =>
new Readable({
read() {
this.push(Buffer.from(content));
this.push(null);
},
});
(zlib.createGzip as sinon.SinonStub).callsFake(() => new PassThrough());
(fs.createReadStream as sinon.SinonStub).callsFake((filePath: string) => {
if (filePath.includes("file1.txt")) {
return createMockStream("hash1");
}
if (filePath.includes("file2.txt")) {
return createMockStream("hash2");
}
return new PassThrough();
});1 participant
Description
Going through my usual exercise of trying out new AI by having it write unit tests - this time, AGY wrote tests for the file uploader used for hosting deploy.
This is part of a larger effort to increase the test coverage for this repo.