test: fix test cases for uncompress stream #118
Conversation
WalkthroughTest modifications refactoring stream handling in the uncompress functionality: file entries transition from direct stream end handling to promise-based pipeline writes with improved error propagation, while directory entries gain explicit stream end listeners for proper sequencing and assertion patterns include descriptive error messages. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @bytemain, 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 focuses on refining the test suite for the uncompress stream functionality. It introduces more explicit and informative assertion messages for directory comparison tests and upgrades the stream processing logic within these tests to leverage 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 improves test assertions and stream handling as described. The use of pipelinePromise instead of stream.pipe is a great improvement for error handling. However, these improvements have been applied inconsistently. The improved assertions are only used for some checks, and the new stream handling logic is only applied to two of several test cases that have identical logic. I've left specific comments with suggestions to apply these improvements consistently across the test file and to consider refactoring duplicated code.
There was a problem hiding this comment.
Pull Request Overview
This PR improves test quality for the uncompress stream functionality by enhancing assertions and fixing stream handling issues. The changes address potential race conditions in file writing and provide better debugging information through descriptive error messages.
Key changes:
- Enhanced assertion messages using
assert.equal()with descriptive failure messages for better test debugging - Fixed stream handling for file writes by using
pipelinePromisewith proper error handling via.catch(done) - Corrected event listener placement by moving
stream.on('end', next)inside the directory-only branch to avoid callingnext()twice for files
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/zip/uncompress_stream.test.js (2)
40-46: Potential double-callback in directory handling.The pattern registers
stream.on('end', next)at line 45 before thefs.mkdircallback completes. Ifmkdirfails,done(err)is called at line 42, but the stream may still emit'end'later and invokenext(), leading to multiple invocations of the done callback.Consider moving the 'end' listener inside the mkdir callback after resuming the stream, or consume the stream even on error:
} else { // directory fs.mkdir(path.join(destDir, header.name), { recursive: true }, err => { - if (err) return done(err); - stream.resume(); + if (err) { + stream.resume(); // consume stream even on error + return done(err); + } + stream.on('end', next); + stream.resume(); }); - stream.on('end', next); }
227-230: Inconsistent assertion pattern - not updated.Unlike similar tests in this file (lines 68-71, 101-104, 134-137, 196-197), these assertions were not updated to use
assert.equalwith descriptive messages. For consistency, these should follow the same pattern.- assert(res.distinct === 0); - assert(res.equal === 4); - assert(res.totalFiles === 4); - assert(res.totalDirs === 0); + assert.equal(res.distinct, 0, 'distinct files count mismatch'); + assert.equal(res.equal, 4, 'equal files count mismatch'); + assert.equal(res.totalFiles, 4, 'total files count mismatch'); + assert.equal(res.totalDirs, 0, 'total directories count mismatch');
♻️ Duplicate comments (7)
test/zip/uncompress_stream.test.js (7)
80-86: Potential double-callback in directory handling.Same issue as flagged earlier: the 'end' listener at line 85 is registered before
mkdircompletes, which could result in calling bothdone(err)andnext()if the directory creation fails.
100-100: Remove debug console.log statement.This debug statement should be removed from the test code.
- console.log(names);
113-119: Potential double-callback in directory handling.Same issue as flagged earlier: registering the 'end' listener before
mkdircompletes could cause double-callback if directory creation fails.
133-133: Remove debug console.log statement.This debug statement should be removed from the test code.
- console.log(names);
146-152: Potential double-callback in directory handling.Same issue as flagged earlier: registering the 'end' listener before
mkdircompletes could cause double-callback if directory creation fails.
208-214: Potential double-callback in directory handling.Same issue as flagged earlier: registering the 'end' listener before
mkdircompletes could cause double-callback if directory creation fails.
239-245: Potential double-callback in directory handling.Same issue as flagged earlier: registering the 'end' listener before
mkdircompletes could cause double-callback if directory creation fails.
🧹 Nitpick comments (1)
test/zip/uncompress_stream.test.js (1)
67-67: Remove debug console.log statement.This debug statement should be removed from the test code.
- console.log(names);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/zip/uncompress_stream.test.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/zip/uncompress_stream.test.js (1)
test/util.js (2)
stream(1-1)pipelinePromise(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Node.js / Test (windows-latest, 24)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18)
- GitHub Check: Agent
2 participants
Improve test assertions for file comparison and enhance stream handling in the uncompress functionality.
Summary by CodeRabbit