git.ts: extract spawn logic into a wrapper and add tests

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]>

Author: dennisameling

src/__tests__/git.test.ts

@@ -0,0 +1,88 @@
+import * as fs from 'fs'
+import * as git from '../git'
+import * as spawn from '../spawn'
+import * as core from '@actions/core'
+
+// We want to mock only the rmSync method on the fs module, and leave everything
+// else untouched.
+jest.mock('fs', () => ({
+  ...jest.requireActual('fs'),
+  rmSync: jest.fn()
+}))
+
+describe('git', () => {
+  // We don't want to _actually_ spawn external commands, so we mock the function
+  let spawnSpy: jest.SpyInstance
+  // Capture the startGroup calls
+  let coreSpy: jest.SpyInstance
+  // The script calls fs.rmSync, so let's mock it and verify it was called
+  let rmSyncSpy: jest.SpyInstance
+
+  beforeEach(() => {
+    coreSpy = jest.spyOn(core, 'startGroup')
+    spawnSpy = jest.spyOn(spawn, 'spawnAndWaitForExitCode').mockResolvedValue({
+      // 0 is the exit code for success
+      exitCode: 0
+    })
+    // We don't want to _actually_ clone the repo, so we mock the function
+    jest.spyOn(git, 'clone').mockResolvedValue()
+    rmSyncSpy = fs.rmSync as jest.Mocked<typeof fs>['rmSync']
+  })
+
+  test('getViaGit build-installers x86_64', async () => {
+    const flavor = 'build-installers'
+    const architecture = 'x86_64'
+    const outputDirectory = 'outputDirectory'
+    const {artifactName, download} = await git.getViaGit(flavor, architecture)
+
+    expect(artifactName).toEqual('git-sdk-64-build-installers')
+
+    await download(outputDirectory, true)
+
+    expect(coreSpy).toHaveBeenCalledWith(`Creating ${flavor} artifact`)
+    expect(spawnSpy).toHaveBeenCalledWith(
+      expect.stringContaining('/bash.exe'),
+      expect.arrayContaining([
+        '.tmp/build-extra/please.sh',
+        'create-sdk-artifact',
+        `--architecture=${architecture}`,
+        `--out=${outputDirectory}`
+      ]),
+      expect.objectContaining({
+        env: expect.objectContaining({
+          // We want to ensure that the correct /bin folders are in the PATH,
+          // so that please.sh can find git.exe
+          // https://github.com/git-for-windows/setup-git-for-windows-sdk/issues/951
+          PATH:
+            expect.stringContaining('/clangarm64/bin') &&
+            expect.stringContaining('/mingw64/bin')
+        })
+      })
+    )
+    expect(rmSyncSpy).toHaveBeenCalledWith('.tmp', {recursive: true})
+  })
+
+  test('getViaGit full x86_64', async () => {
+    const flavor = 'full'
+    const architecture = 'x86_64'
+    const outputDirectory = 'outputDirectory'
+    const {artifactName, download} = await git.getViaGit(flavor, architecture)
+
+    expect(artifactName).toEqual('git-sdk-64-full')
+
+    await download(outputDirectory, true)
+
+    expect(coreSpy).toHaveBeenCalledWith(`Checking out git-sdk-64`)
+    expect(spawnSpy).toHaveBeenCalledWith(
+      expect.stringContaining('/git.exe'),
+      expect.arrayContaining([
+        '--git-dir=.tmp',
+        'worktree',
+        'add',
+        outputDirectory
+      ]),
+      expect.any(Object)
+    )
+    expect(rmSyncSpy).toHaveBeenCalledWith('.tmp', {recursive: true})
+  })
+})

src/git.ts

@@ -1,5 +1,5 @@
 import * as core from '@actions/core'
-import {ChildProcess, spawn} from 'child_process'
+import {spawnAndWaitForExitCode, SpawnReturnArgs} from './spawn'
 import {Octokit} from '@octokit/rest'
 import {delimiter} from 'path'
 import * as fs from 'fs'
@@ -48,14 +48,14 @@ export function getArtifactMetadata(
   return {repo, artifactName}
 }
 
-async function clone(
+export async function clone(
   url: string,
   destination: string,
   verbose: number | boolean,
   cloneExtraOptions: string[] = []
 ): Promise<void> {
   if (verbose) core.info(`Cloning ${url} to ${destination}`)
-  const child = spawn(
+  const child = await spawnAndWaitForExitCode(
     gitExePath,
     [
       'clone',
@@ -69,44 +69,30 @@ async function clone(
     {
       env: {
         GIT_CONFIG_PARAMETERS
-      },
-      stdio: [undefined, 'inherit', 'inherit']
+      }
     }
   )
-  return new Promise<void>((resolve, reject) => {
-    child.on('close', code => {
-      if (code === 0) {
-        resolve()
-      } else {
-        reject(new Error(`git clone: exited with code ${code}`))
-      }
-    })
-  })
+  if (child.exitCode !== 0) {
+    throw new Error(`git clone: exited with code ${child.exitCode}`)
+  }
 }
 
 async function updateHEAD(
   bareRepositoryPath: string,
   headSHA: string
 ): Promise<void> {
-  const child = spawn(
+  const child = await spawnAndWaitForExitCode(
     gitExePath,
     ['--git-dir', bareRepositoryPath, 'update-ref', 'HEAD', headSHA],
     {
       env: {
         GIT_CONFIG_PARAMETERS
-      },
-      stdio: [undefined, 'inherit', 'inherit']
+      }
     }
   )
-  return new Promise<void>((resolve, reject) => {
-    child.on('close', code => {
-      if (code === 0) {
-        resolve()
-      } else {
-        reject(new Error(`git: exited with code ${code}`))
-      }
-    })
-  })
+  if (child.exitCode !== 0) {
+    throw new Error(`git: exited with code ${child.exitCode}`)
+  }
 }
 
 export async function getViaGit(
@@ -175,17 +161,16 @@ export async function getViaGit(
       ])
       core.endGroup()
 
-      let child: ChildProcess
+      let child: SpawnReturnArgs
       if (flavor === 'full') {
         core.startGroup(`Checking out ${repo}`)
-        child = spawn(
+        child = await spawnAndWaitForExitCode(
           gitExePath,
           [`--git-dir=.tmp`, 'worktree', 'add', outputDirectory, head_sha],
           {
             env: {
               GIT_CONFIG_PARAMETERS
-            },
-            stdio: [undefined, 'inherit', 'inherit']
+            }
           }
         )
       } else {
@@ -200,7 +185,7 @@ export async function getViaGit(
 
         core.startGroup(`Creating ${flavor} artifact`)
         const traceArg = verbose ? ['-x'] : []
-        child = spawn(
+        child = await spawnAndWaitForExitCode(
           `${gitForWindowsUsrBinPath}/bash.exe`,
           [
             ...traceArg,
@@ -221,21 +206,16 @@ export async function getViaGit(
               CHERE_INVOKING: '1',
               MSYSTEM: 'MINGW64',
               PATH: `${gitForWindowsBinPaths.join(delimiter)}${delimiter}${process.env.PATH}`
-            },
-            stdio: [undefined, 'inherit', 'inherit']
+            }
           }
         )
       }
-      return new Promise<void>((resolve, reject) => {
-        child.on('close', code => {
-          core.endGroup()
-          if (code === 0) {
-            fs.rm('.tmp', {recursive: true}, () => resolve())
-          } else {
-            reject(new Error(`process exited with code ${code}`))
-          }
-        })
-      })
+      core.endGroup()
+      if (child.exitCode === 0) {
+        fs.rmSync('.tmp', {recursive: true})
+      } else {
+        throw new Error(`process exited with code ${child.exitCode}`)
+      }
     }
   }
 }

src/spawn.ts

@@ -0,0 +1,28 @@
+import {spawn as SpawnInternal, SpawnOptions} from 'child_process'
+
+export type SpawnReturnArgs = {
+  exitCode: number | null
+}
+
+/**
+ * Simple wrapper around NodeJS's "child_process.spawn" function.
+ * Since we only use the exit code, we only expose that.
+ */
+export async function spawnAndWaitForExitCode(
+  command: string,
+  args: readonly string[],
+  options: SpawnOptions
+): Promise<SpawnReturnArgs> {
+  const child = SpawnInternal(command, args, {
+    ...options,
+    // 'inherit' means that the child process will use the same stdio/stderr as the parent process
+    stdio: [undefined, 'inherit', 'inherit']
+  })
+
+  return new Promise<SpawnReturnArgs>((resolve, reject) => {
+    child.on('error', reject)
+    child.on('close', code => {
+      resolve({exitCode: code})
+    })
+  })
+}