Extract spawn logic into a wrapper and add tests #953
Conversation
dennisameling
force-pushed
the
spawn-logic-and-path-issue
branch
2 times, most recently
from
1b5d636
to
c969170
Compare
src/__tests__/git.test.ts
Outdated
| // We don't want to _actually_ clone the repo, so we mock the function | ||
| jest.spyOn(git, 'clone').mockResolvedValue() | ||
|
|
||
| // The script removes the .tmp folder at the end, so let's create it |
There was a problem hiding this comment.
Can we mock mkdirSync()/rmdirSync() instead and verify that they were called?
src/spawn.ts
Outdated
| * Simple wrapper around NodeJS's "child_process.spawn" function. | ||
| * Since we only use the exit code, we only expose that. | ||
| */ | ||
| export async function spawn( |
There was a problem hiding this comment.
Can we rename this to spawnAndWaitForExitCode()? I find it a bit confusing that spawn() shouldn't return immediately upon spawning the process without waiting for it to finish, as that would be the semantics I've come to expect of any function called spawn()...
src/spawn.ts
Outdated
| export async function spawn( | ||
| command: string, | ||
| args: readonly string[], | ||
| options: SpawnOptionsWithStdioTuple<StdioPipe, StdioNull, StdioNull> |
There was a problem hiding this comment.
We have two callers that both pass in [undefined, 'inherit', 'inherit'] here. How about hard-coding it and dropping the options function parameter?
There was a problem hiding this comment.
Great suggestion, thanks! I couldn't drop options completely because the callers are still passing env objects, but I was able to change SpawnOptionsWithStdioTuple<StdioPipe, StdioNull, StdioNull> to simply SpawnOptions by hard-coding stdio 👍🏼
Currently, it's a bit hard to ensure that the code in git.exe actually does what we expect it to do, especially considering that we're calling things like please.sh and assume that some executables are available on the PATH. Let's ensure we add some tests for these code paths, which we can easily extend with more tests if needed. Signed-off-by: Dennis Ameling <[email protected]>
Signed-off-by: Dennis Ameling <[email protected]>
dennisameling
force-pushed
the
spawn-logic-and-path-issue
branch
from
c969170
to
9d4ed59
Compare
Currently, it's a bit hard to ensure that the code in git.exe actually does what we expect it to do, especially considering that we're calling things like please.sh and assume that some executables are available on the PATH.
Let's ensure we add some tests for these code paths, which we can easily extend with more tests if needed.