regression-tests: update custom mocha reporter to exit rather than mocha itself (#41266)

* update custom mocha reporter to be more robust

- ensure all files reference by mocha and mocha reporter are relative to
  repo root
- remove `--exit` from `yarn run test:regression`. The same
  functionality is now done by the customMochaSpecReporter with
  `process.exit(1)`
- some quality of life things in the qa test runner script

* Update client/shared/dev/customMochaSpecReporter.js

Co-authored-by: Valery Bugakov <skymk1@gmail.com>

* Update client/shared/dev/customMochaSpecReporter.js

Co-authored-by: Valery Bugakov <skymk1@gmail.com>

* prettier things

* use filenames in annotation

Co-authored-by: Valery Bugakov <skymk1@gmail.com>

.mocharc.js

@@ -1,11 +1,15 @@
+const { execSync } = require('child_process')
+
+const repoRoot = execSync('git rev-parse --show-toplevel').toString().trimEnd()
+
 module.exports = {
   require: [
     'ts-node/register/transpile-only',
     'abort-controller/polyfill',
-    __dirname + '/client/shared/dev/fetch',
-    __dirname + '/client/shared/dev/suppressPollyErrors',
+    repoRoot + '/client/shared/dev/fetch',
+    repoRoot + '/client/shared/dev/suppressPollyErrors',
   ],
-  reporter: __dirname + '/client/shared/dev/customMochaSpecReporter.js',
+  reporter: repoRoot + '/client/shared/dev/customMochaSpecReporter.js',
   extension: ['js', 'ts'],
   // 1 minute test timeout. This must be greater than the default Puppeteer
   // command timeout of 30s in order to get the stack trace to point to the

client/shared/dev/customMochaSpecReporter.js

@@ -1,6 +1,8 @@
+const { execSync } = require('child_process')
 const { Console } = require('console')
 const fs = require('fs')
 
+const repoRoot = execSync('git rev-parse --show-toplevel').toString().trimEnd()
 const mocha = require('mocha')
 
 const originalConsoleLog = mocha.reporters.Base.consoleLog
@@ -20,7 +22,6 @@ class SpecFileReporter extends mocha.reporters.Spec {
   constructor(runner, options) {
     super(runner, options)
     this.title = 'placeholder'
-    this.buildkite = false
 
     if ('BUILDKITE' in process.env) {
       this.buildkite = true
@@ -29,16 +30,26 @@ class SpecFileReporter extends mocha.reporters.Spec {
     }
 
     if ('BUILDKITE_LABEL' in process.env) {
-      this.title = process.env.BUILDKIATE_LABEL
+      this.title = process.env.BUILDKITE_LABEL || 'placeholder'
     }
 
     if (this.buildkite === true && typeof process.env.BUILDKITE_LABEL === undefined) {
-      console.warn(
-        `In Buildkite but BUILDKITE_LABEL not found in environment. Using title '${this.title || 'placeholder'}'`
-      )
+      console.info(`In Buildkite but BUILDKITE_LABEL not found in environment. Using title '${this.title}'`)
     }
   }
 
+  safeClose(stream) {
+    return new Promise((resolve, reject) => {
+      stream.close(error => {
+        if (error) {
+          reject(error)
+        }
+
+        resolve()
+      })
+    })
+  }
+
   epilogue() {
     // We first let mocha.reporters.Spec do it's usual reporting using the default console defined on Base
     // Which means the report will be written to the terminal
@@ -46,9 +57,11 @@ class SpecFileReporter extends mocha.reporters.Spec {
 
     // We only output the epilogue to a file when we're in BUILDKITE and there are failures
     if (this.buildkite === true && this.failures.length > 0) {
+      const file = fs.createWriteStream(`${repoRoot}/annotations/${this.title}`)
       const customConsole = new Console({
-        stdout: fs.createWriteStream(`./annotations/mocha-test-output-${this.title || 'placeholder'}`),
+        stdout: file,
       })
+
       // We now want the Spec reporter (aka epilogue) to be written to a file, but Spec uses the console defined on Base!
       // So we swap out the consoleLog defined on Base with our customLog one
       // https://sourcegraph.com/github.com/mochajs/mocha/-/blob/lib/reporters/base.js?L43:5
@@ -58,8 +71,23 @@ class SpecFileReporter extends mocha.reporters.Spec {
       // https://mochajs.org/api/reporters_base.js.html#line367
       super.epilogue()
       // The report has been written to a file, so now we swap the consoleLog back to the originalConsole logger
-      // eslint-disable-next-line @typescript-eslint/unbound-method
       mocha.reporters.Base.consoleLog = originalConsoleLog
+      // We want to make sure before this reporter exits that the data written to file have been flushed
+      // In some scenarios, the node process exits too quickly and the data hasn't been flushed to the file yet
+      this.safeClose(file)
+        .then(() => {
+          const path = file.path.toString()
+          console.log(`${path} successfully closed`)
+        })
+        .catch(error => console.error(error))
+        .finally(() => {
+          console.warn('force exiting the process after writing report to file')
+          // This performs the same function as passing --exit to the mocha test runner
+          // When the regression tests run, some resources are not properly cleaned up. Leading to
+          // the test runner just hanging since it is waiting on an open resource to exit.
+          // TODO(burmudar): hunt this resource down
+          process.exit(1)
+        })
     }
   }
 }

client/web/package.json

@@ -6,8 +6,8 @@
   "license": "Apache-2.0",
   "scripts": {
     "test": "yarn run -T jest --testPathIgnorePatterns end-to-end --testPathIgnorePatterns regression integration",
-    "task:mocha": "yarn run -T cross-env TS_NODE_COMPILER_OPTIONS='{\"module\":\"commonjs\"}' mocha",
-    "test:regression": "yarn run task:mocha './src/regression/**/*.test.ts' --exit",
+    "task:mocha": "yarn run -T cross-env TS_NODE_COMPILER_OPTIONS='{\"module\":\"commonjs\"}' mocha --config ../../.mocharc.js",
+    "test:regression": "yarn run task:mocha './src/regression/**/*.test.ts'",
     "test:regression:codeintel": "yarn task:mocha ./src/regression/codeintel.test.ts",
     "test:regression:config-settings": "yarn task:mocha ./src/regression/config-settings.test.ts",
     "test:regression:core": "yarn task:mocha ./src/regression/core.test.ts",

dev/ci/integration/qa/test.sh

@@ -3,7 +3,13 @@
 export SOURCEGRAPH_BASE_URL="${1:-"http://localhost:7080"}"
 
 # shellcheck disable=SC1091
-source /root/.profile
+if [[ $(id -u) -eq 1 ]]; then
+    source /root/.profile
+else
+    # shellcheck disable=SC1090
+    source "${HOME}/.profile"
+fi
+
 cd "$(dirname "${BASH_SOURCE[0]}")/../../../.."
 
 set -e
@@ -16,11 +22,16 @@ popd
 # Load variables set up by init-server, disabling `-x` to avoid printing variables
 set +x
 # shellcheck disable=SC1091
-source /root/.sg_envrc
+if [[ $(id -u) -eq 1 ]]; then
+    source /root/.sg_envrc
+else
+    # shellcheck disable=SC1090
+    source "${HOME}/.sg_envrc"
+fi
 
 echo "--- TEST: Checking Sourcegraph instance is accessible"
-curl -f http://localhost:7080
-curl -f http://localhost:7080/healthz
+curl -f "${SOURCEGRAPH_BASE_URL}"
+curl -f "${SOURCEGRAPH_BASE_URL}/healthz"
 echo "--- TEST: Running tests"
 # Run all tests, and error if one fails
 test_status=0

enterprise/dev/ci/internal/ci/operations.go

@@ -327,7 +327,12 @@ func clientIntegrationTests(pipeline *bk.Pipeline) {
 			// If PERCY_PARALLEL_TOTAL is set, the API will wait for that many finalized builds to finalize the Percy build.
 			// https://docs.percy.io/docs/parallel-test-suites#how-it-works
 			bk.Env("PERCY_PARALLEL_TOTAL", strconv.Itoa(parallelTestCount)),
-			bk.Cmd(fmt.Sprintf(`dev/ci/yarn-web-integration.sh "%s"`, chunkTestFiles)),
+			bk.AnnotatedCmd(fmt.Sprintf(`dev/ci/yarn-web-integration.sh "%s"`, chunkTestFiles), bk.AnnotatedCmdOpts{
+				Annotations: &bk.AnnotationOpts{
+					IncludeNames:    true,
+					MultiJobContext: "puppeteer",
+				},
+			}),
 			bk.ArtifactPaths("./puppeteer/*.png"))
 	}
 }