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

Author: dennisameling

.jest/setEnvVars.ts

@@ -0,0 +1,5 @@
+/*
+This ensures that the PATH of the machine that the tests are running on,
+doesn't leak into our tests.
+*/
+process.env.PATH = '' 

jest.config.js

@@ -1,6 +1,7 @@
 module.exports = {
   clearMocks: true,
   moduleFileExtensions: ['js', 'ts'],
+  setupFiles: ["<rootDir>/.jest/setEnvVars.ts"],
   testEnvironment: 'node',
   testMatch: ['**/*.test.ts'],
   testRunner: 'jest-circus/runner',

src/__tests__/git.test.ts

@@ -0,0 +1,86 @@
+import {mkdirSync, rmdirSync, existsSync} from 'fs'
+import * as git from '../git'
+import * as spawn from '../spawn'
+import * as core from '@actions/core'
+
+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
+
+  beforeEach(() => {
+    coreSpy = jest.spyOn(core, 'startGroup')
+    spawnSpy = jest.spyOn(spawn, 'spawn').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()
+
+    // The script removes the .tmp folder at the end, so let's create it
+    mkdirSync('.tmp')
+  })
+
+  afterEach(() => {
+    // Clean up the .tmp folder if the script didn't already do it
+    if (existsSync('.tmp')) {
+      rmdirSync('.tmp', {recursive: true})
+    }
+  })
+
+  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')
+        })
+      })
+    )
+  })
+
+  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)
+    )
+  })
+})

src/git.ts

@@ -1,5 +1,5 @@
 import * as core from '@actions/core'
-import {ChildProcess, spawn} from 'child_process'
+import {spawn, 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 spawn(
     gitExePath,
     [
       'clone',
@@ -73,22 +73,16 @@ async function clone(
       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 spawn(
     gitExePath,
     ['--git-dir', bareRepositoryPath, 'update-ref', 'HEAD', headSHA],
     {
@@ -98,15 +92,9 @@ async function updateHEAD(
       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,10 +163,10 @@ export async function getViaGit(
       ])
       core.endGroup()
 
-      let child: ChildProcess
+      let child: SpawnReturnArgs
       if (flavor === 'full') {
         core.startGroup(`Checking out ${repo}`)
-        child = spawn(
+        child = await spawn(
           gitExePath,
           [`--git-dir=.tmp`, 'worktree', 'add', outputDirectory, head_sha],
           {
@@ -200,7 +188,7 @@ export async function getViaGit(
 
         core.startGroup(`Creating ${flavor} artifact`)
         const traceArg = verbose ? ['-x'] : []
-        child = spawn(
+        child = await spawn(
           `${gitForWindowsUsrBinPath}/bash.exe`,
           [
             ...traceArg,
@@ -226,16 +214,12 @@ export async function getViaGit(
           }
         )
       }
-      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,29 @@
+import {
+  spawn as SpawnInternal,
+  SpawnOptionsWithStdioTuple,
+  StdioNull,
+  StdioPipe
+} 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 spawn(
+  command: string,
+  args: readonly string[],
+  options: SpawnOptionsWithStdioTuple<StdioPipe, StdioNull, StdioNull>
+): Promise<SpawnReturnArgs> {
+  const child = SpawnInternal(command, args, options)
+
+  return new Promise<SpawnReturnArgs>((resolve, reject) => {
+    child.on('error', reject)
+    child.on('close', code => {
+      resolve({exitCode: code})
+    })
+  })
+}