tests(#468): 37 unit tests for cli/plugins-update.js (checksum diff, disk I/O, tarball pipeline)#326
tests(#468): 37 unit tests for cli/plugins-update.js (checksum diff, disk I/O, tarball pipeline)#326javimosch wants to merge 1 commit into
Conversation
Export diffCatalogs, readLocalCatalog, writeLocalCatalog, downloadTarball, and extractPluginsFromTarball for direct testing. Test coverage: - diffCatalogs (7): null/empty local → all added, identical checksums → unchanged, stale checksum → changed, mixed add+change+unchanged - readLocalCatalog (6): missing file, valid JSON, invalid JSON, null/non-object parsed value, fs.readFileSync throws - writeLocalCatalog (4): creates dir when missing, skips mkdirSync when dir exists, writes to correct path, trailing newline - downloadTarball (5): spawnSync error, non-zero exit, missing archive, zero-byte archive, success path - extractPluginsFromTarball (7): creates destDir, correct tar args, tolerates exit 2, throws on exit 1, throws on spawnSync error, batches >200 into 2 calls, 200 uses 1 call - updatePlugins (8): check=true skips download, up-to-date skips download, force=true ignores local catalog, stale catalog triggers full update+write, tmpDir cleaned via finally even on error, remote_count in result, fetch failure throws, invalid catalog format throws Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe module now exports plugin update helper functions alongside ChangesPlugin update workflow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Review — Head of Org Engineering: The PR adds a new test file (approved — review-only mode; merge when ready.) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/plugins-update.js (1)
204-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep test-only helpers off the top-level public API.
Exporting these helpers alongside
updatePluginsturns internal implementation details into supported module surface. That makes future refactors of catalog I/O and tar handling externally breaking for consumers, even though they were only exposed for tests. Consider hanging them off a clearly-internal namespace instead.♻️ Possible shape
module.exports = { updatePlugins, - diffCatalogs, - readLocalCatalog, - writeLocalCatalog, - downloadTarball, - extractPluginsFromTarball, + _internal: { + diffCatalogs, + readLocalCatalog, + writeLocalCatalog, + downloadTarball, + extractPluginsFromTarball, + }, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/plugins-update.js` around lines 204 - 211, The top-level export object in updatePlugins is exposing internal test-only helpers as public API, so move those helpers off the main module surface. Keep updatePlugins as the primary export and place diffCatalogs, readLocalCatalog, writeLocalCatalog, downloadTarball, and extractPluginsFromTarball under a clearly internal namespace or equivalent internal-only export path, so consumers only depend on supported functionality.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cli/plugins-update.js`:
- Around line 204-211: The top-level export object in updatePlugins is exposing
internal test-only helpers as public API, so move those helpers off the main
module surface. Keep updatePlugins as the primary export and place diffCatalogs,
readLocalCatalog, writeLocalCatalog, downloadTarball, and
extractPluginsFromTarball under a clearly internal namespace or equivalent
internal-only export path, so consumers only depend on supported functionality.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8992585c-08f1-48a2-b73c-4e8c37db8c04
📒 Files selected for processing (2)
__tests__/plugins-update.test.jscli/plugins-update.js
1 participant
What changed
Export five private helpers from
cli/plugins-update.jsfor direct testing, then add__tests__/plugins-update.test.jswith 37 unit tests covering all exported functions.Why
plugins-update.jshad zero test coverage. The module owns the complete plugin-update pipeline: catalog diff (checksum-based stale detection), disk read/write, tarball download, and extraction. Each of those paths has real error branches that were untested.Test coverage added
diffCatalogspluginskey; empty remotereadLocalCatalogreadFileSyncthrows → nullwriteLocalCatalogmkdirSyncwhen dir exists; writes to correct path; trailing newlinedownloadTarballspawnSyncerror →integration_error; non-zero exit → throws; missing archive → throws; zero-byte archive → throws; success → returns pathextractPluginsFromTarballdestDir; correcttarargs; tolerates exit 2 (some paths missing); exit 1 → throws;spawnSyncerror → throws; >200 plugins batched into 2 calls; exactly 200 → 1 callupdatePluginscheck=trueskips download; up-to-date skips download;force=trueignores local catalog; stale catalog triggers full update+write;tmpDircleaned viafinallyeven on error;remote_countin result; fetch failure throws withcode: 105; invalid remote catalog format throwsVerification
Closes #468 (mago task #468)
Summary by CodeRabbit