test(Docker): cover getDockerCredentials() paths - #3448
Draft
Dave Shoup (shouples) wants to merge 1 commit into
Draft
test(Docker): cover getDockerCredentials() paths#3448Dave Shoup (shouples) wants to merge 1 commit into
getDockerCredentials() paths#3448Dave Shoup (shouples) wants to merge 1 commit into
Conversation
`execSync` was imported straight from `child_process`, so Sinon could not intercept it and `getDockerCredentials()` shipped untested (deferred from PR #1940). Route it through a new `fsWrappers.execSync` wrapper and add coverage for the cache hit, missing/unreadable config, happy path, store prefix dedup, command failure, invalid creds, and non-JSON output paths. Also move the credential `JSON.parse()` inside the command try/catch so a helper emitting non-JSON on stdout is handled like any other failure (log and return undefined) rather than throwing to the caller. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds unit test coverage for getDockerCredentials() (Docker credential helper lookup + SecretStorage caching) by introducing a stubbable execSync wrapper and hardening one error path so non-JSON helper stdout is handled gracefully.
Changes:
- Added a thin
execSyncwrapper insrc/utils/fsWrappers.tsto enable Sinon stubbing. - Updated
getDockerCredentials()to use the wrapper and to performJSON.parse()inside the commandtry/catch. - Added a focused
getDockerCredentials()unit test suite covering cache, config/read failures, helper invocation behavior, and invalid/non-JSON outputs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/utils/fsWrappers.ts | Adds a stubbable execSync wrapper returning UTF-8 decoded output as a string. |
| src/docker/credentials.ts | Routes credential helper execution through the wrapper and catches non-JSON helper stdout by parsing inside try/catch. |
| src/docker/credentials.test.ts | Adds an 8-case test suite covering getDockerCredentials() control-flow branches and caching behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.




Summary of Changes
getDockerCredentials()(which reads the local Docker credential helper so the extension can authenticate to a private registry) shipped in #1940 without unit tests, because it calledexecSyncimported straight fromchild_processand Sinon can only stub module exports. This PR closes that gap.execSyncwrapper tosrc/utils/fsWrappers.ts(the same modulegetDockerCredentials()already importsreadFileSyncfrom) and route the credential lookup through it.getDockerCredentials()test suite covering all eight branches: cache hit, missing/unreadable Docker config, nocredsStoreconfigured, the happy path (query + validate + cache), thedocker-credential-prefix dedup, command failure, invalid credentials, and non-JSON output.JSON.parse()of the helper's stdout now runs inside the commandtry/catch, so a helper that prints a plain-text error instead of JSON is handled like any other failure (log and returnundefined) rather than throwing to the caller.Click-testing instructions
Not applicable - this PR only adds tests and hardens one error path (see above), so there's nothing to click-test. Verified via the unit tests noted in the section below.
Optional: Any additional details or context that should be provided?
fsWrappers.tsper the issue title. That module is already the home for stubbable native (C-backed) wrappers, not strictly filesystem ones - it also wrapsos.tmpdir()- so its doc comment was broadened to say so. A separate, sidecar-specificchild_process.spawn()wrapper exists insrc/sidecar/utils.ts; consolidating the twochild_processwrappers into one home is a reasonable follow-up but out of scope here.execSyncalways decodes as UTF-8 and returns astring, soencodingis intentionally not a caller-overridable option (Omit<ExecSyncOptions, "encoding">), keeping the: stringreturn type honest.npx gulp check,npx gulp lint(0 errors), andnpx gulp test -t "credentials"all pass - 30 tests, including the 8 newgetDockerCredentials()cases.Closes #1941
Pull request checklist
Please check if your PR fulfills the following (if applicable):
Tests
Release notes