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 👍🏼
dennisameling
force-pushed
the
spawn-logic-and-path-issue
branch
from
c969170 to
9d4ed59
Compare
dscho
force-pushed
the
spawn-logic-and-path-issue
branch
from
9d4ed59 to
36d7790
Compare
Signed-off-by: Dennis Ameling <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
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: Johannes Schindelin <[email protected]>
dscho
force-pushed
the
spawn-logic-and-path-issue
branch
from
36d7790 to
54bd741
Compare
Signed-off-by: Johannes Schindelin <[email protected]>
dscho
force-pushed
the
spawn-logic-and-path-issue
branch
from
54bd741 to
a806009
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.