chore: add execProcess wrapper and refactor poetryUtils (Fixes #1239)#1240
chore: add execProcess wrapper and refactor poetryUtils (Fixes #1239)#1240karthiknadig wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves testability and consistency of child-process usage by introducing an execProcess() wrapper in src/common/childProcess.apis.ts and refactoring Poetry utilities to use it, while also fixing Poetry 1.x version parsing.
Changes:
- Added
execProcess()wrapper aroundcp.exec()insrc/common/childProcess.apis.ts. - Refactored
src/managers/poetry/poetryUtils.tsto useexecProcess()instead of a directchild_processimport + promisify. - Added unit tests covering
getPoetryVersion()parsing and demonstrating stubbingexecProcess().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/common/childProcess.apis.ts | Adds execProcess() wrapper to centralize cp.exec usage and improve mockability. |
| src/managers/poetry/poetryUtils.ts | Switches Poetry exec calls to the wrapper and adjusts version-regex parsing for Poetry 1.x output. |
| src/test/managers/poetry/poetryUtils.unit.test.ts | Adds unit tests that stub childProcess.apis.execProcess() and validate version parsing. |
| return { | ||
| stdout: result.stdout ?? '', | ||
| stderr: result.stderr ?? '', |
There was a problem hiding this comment.
execProcess is documented/typed to return stdout/stderr as strings, but cp.exec can return Buffers when callers pass encoding: 'buffer' | null in options. As written, a Buffer will flow through (it’s not nullish) and can break callers that do string operations (e.g. .trim()). Consider forcing encoding: 'utf8' (or converting Buffer→string before returning) to make the wrapper’s contract true and avoid runtime type mismatches.
| return { | |
| stdout: result.stdout ?? '', | |
| stderr: result.stderr ?? '', | |
| const rawStdout = result.stdout ?? ''; | |
| const rawStderr = result.stderr ?? ''; | |
| return { | |
| stdout: Buffer.isBuffer(rawStdout) ? rawStdout.toString('utf8') : rawStdout, | |
| stderr: Buffer.isBuffer(rawStderr) ? rawStderr.toString('utf8') : rawStderr, |
This PR adds an
execProcesswrapper tochildProcess.apis.tsand refactorspoetryUtils.tsto use it, improving testability.Changes:
execProcessfunction tosrc/common/childProcess.apis.tsas a wrapper aroundcp.execwithPYTHONUTF8handlingsrc/managers/poetry/poetryUtils.tsto useexecProcessinstead of directchild_processimportgetPoetryVersionthat didn't handle the space in Poetry 1.x version output formatpoetryUtils.unit.test.tsdemonstrating the mocking pattern forexecProcessTesting:
Note: This is a partial implementation of #1239. The acceptance criteria for auditing
fsimports is deferred to a future PR to keep this change focused.For #1239