reviewstack: fix verify addons/ folder github ci

.github/workflows/sapling-addons.yml

@@ -14,6 +14,8 @@ jobs:
     container:
       image: ${{ format('ghcr.io/{0}/build_ubuntu_20_04:latest', github.repository) }}
     steps:
+      - name: Install fb-watchman
+        run: curl -fsSL https://github.com/facebook/watchman/releases/download/v2023.09.18.00/watchman_ubuntu20.04_v2023.09.18.00.deb -o watchman.deb && apt -y -f install ./watchman.deb
       - name: Checkout Code
         uses: actions/checkout@v3
       - name: Grant Access

addons/isl-server/src/Repository.ts

@@ -8,7 +8,6 @@
 import type {CodeReviewProvider} from './CodeReviewProvider';
 import type {ServerSideTracker} from './analytics/serverSideTracker';
 import type {Logger} from './logger';
-import type {ExecaError} from 'execa';
 import type {
   CommitInfo,
   CommitPhaseType,
@@ -44,7 +43,7 @@ import {WatchForChanges} from './WatchForChanges';
 import {DEFAULT_DAYS_OF_COMMITS_TO_LOAD, ErrorShortMessages} from './constants';
 import {GitHubCodeReviewProvider} from './github/githubCodeReviewProvider';
 import {isGithubEnterprise} from './github/queryGraphQL';
-import {handleAbortSignalOnProcess, serializeAsyncCall} from './utils';
+import {handleAbortSignalOnProcess, isExecaError, serializeAsyncCall} from './utils';
 import execa from 'execa';
 import {CommandRunner} from 'isl/src/types';
 import os from 'os';
@@ -838,8 +837,9 @@ async function runCommand(
   const result = execa(command, args, options);
 
   let timedOut = false;
+  let timeoutId: NodeJS.Timeout | undefined;
   if (timeout > 0) {
-    const timeoutId = setTimeout(() => {
+    timeoutId = setTimeout(() => {
       result.kill('SIGTERM', {forceKillAfterTimeout: 5_000});
       logger.error(`Timed out waiting for ${command} ${args[0]} to finish`);
       timedOut = true;
@@ -862,13 +862,11 @@ async function runCommand(
       }
     }
     throw err;
+  } finally {
+    clearTimeout(timeoutId);
   }
 }
 
-function isExecaError(err: unknown): err is ExecaError {
-  return typeof err === 'object' && err != null && 'exitCode' in err;
-}
-
 export const __TEST__ = {
   runCommand,
 };

addons/isl-server/src/__tests__/analytics.test.ts

@@ -11,12 +11,54 @@ import type {ServerPlatform} from '../serverPlatform';
 
 import {Repository} from '../Repository';
 import {makeServerSideTracker} from '../analytics/serverSideTracker';
+import * as execa from 'execa';
 import {mockLogger} from 'shared/testUtils';
 import {defer} from 'shared/utils';
 
 /** Matches any non-empty string */
 const anyActualString = expect.stringMatching(/.+/);
 
+jest.mock('../WatchForChanges', () => {
+  class MockWatchForChanges {
+    dispose = jest.fn();
+  }
+  return {WatchForChanges: MockWatchForChanges};
+});
+
+jest.mock('execa', () => {
+  return jest.fn();
+});
+
+function mockExeca(
+  cmds: Array<[RegExp, (() => {stdout: string} | Error) | {stdout: string} | Error]>,
+) {
+  return jest.spyOn(execa, 'default').mockImplementation(((cmd: string, args: Array<string>) => {
+    const argStr = cmd + ' ' + args?.join(' ');
+    const execaOther = {
+      kill: jest.fn(),
+      on: jest.fn((event, cb) => {
+        // immediately call exit cb to teardown timeout
+        if (event === 'exit') {
+          cb();
+        }
+      }),
+    };
+    for (const [regex, output] of cmds) {
+      if (regex.test(argStr)) {
+        let value = output;
+        if (typeof output === 'function') {
+          value = output();
+        }
+        if (value instanceof Error) {
+          throw value;
+        }
+        return {...execaOther, ...value};
+      }
+    }
+    return {...execaOther, stdout: ''};
+  }) as unknown as typeof execa.default);
+}
+
 describe('track', () => {
   const mockSendData = jest.fn();
   let tracker: ServerSideTracker;
@@ -61,6 +103,20 @@ describe('track', () => {
   });
 
   it('allows setting repository', () => {
+    // No need to call the actual command lines to test tracking
+    const execaSpy = mockExeca([
+      [/^sl config paths.default/, {stdout: 'https://github.com/facebook/sapling.git'}],
+      [/^sl config github.pull_request_domain/, {stdout: 'github.com'}],
+      [/^sl root --dotdir/, {stdout: '/path/to/myRepo/.sl'}],
+      [/^sl root/, {stdout: '/path/to/myRepo'}],
+      [
+        /^gh auth status --hostname gitlab.myCompany.com/,
+        new Error('not authenticated on this hostname'),
+      ],
+      [/^gh auth status --hostname ghe.myCompany.com/, {stdout: ''}],
+      [/^gh api graphql/, {stdout: '{}'}],
+    ]);
+
     const repo = new Repository(
       {
         type: 'success',
@@ -86,6 +142,7 @@ describe('track', () => {
       mockLogger,
     );
     repo.dispose();
+    execaSpy.mockClear();
   });
 
   it('uses consistent session id, but different track ids', () => {

addons/isl-server/src/github/githubCodeReviewProvider.ts

@@ -115,6 +115,7 @@ export class GitHubCodeReviewProvider implements CodeReviewProvider {
 
   public dispose() {
     this.diffSummaries.removeAllListeners();
+    this.triggerDiffSummariesFetch.dispose();
   }
 
   public getSummaryName(): string {

addons/isl-server/src/github/queryGraphQL.ts

@@ -4,9 +4,7 @@
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  */
-
-import type {ExecaError} from 'execa';
-
+import {isExecaError} from '../utils';
 import execa from 'execa';
 
 export default async function queryGraphQL<TData, TVariables>(
@@ -38,25 +36,27 @@ export default async function queryGraphQL<TData, TVariables>(
   args.push('--hostname', hostname);
   args.push('-f', `query=${query}`);
 
-  const {stdout} = await execa('gh', args, {stdout: 'pipe', stderr: 'pipe'}).catch(
-    (error: ExecaError & {code?: string}) => {
+  try {
+    const {stdout} = await execa('gh', args, {stdout: 'pipe', stderr: 'pipe'});
+    const json = JSON.parse(stdout);
+
+    if (Array.isArray(json.errors)) {
+      return Promise.reject(`Error: ${json.errors[0].message}`);
+    }
+
+    return json.data;
+  } catch (error: unknown) {
+    if (isExecaError(error)) {
       if (error.code === 'ENOENT' || error.code === 'EACCES') {
         // `gh` not installed on path
         throw new Error(`GhNotInstalledError: ${(error as Error).stack}`);
       } else if (error.exitCode === 4) {
         // `gh` CLI exit code 4 => authentication issue
         throw new Error(`NotAuthenticatedError: ${(error as Error).stack}`);
       }
-      throw error;
-    },
-  );
-  const json = JSON.parse(stdout);
-
-  if (Array.isArray(json.errors)) {
-    return Promise.reject(`Error: ${json.errors[0].message}`);
+    }
+    throw error;
   }
-
-  return json.data;
 }
 
 /**

addons/isl-server/src/utils.ts

@@ -165,6 +165,6 @@ export function parseExecJson<T>(
     });
 }
 
-export function isExecaError(s: unknown): s is ExecaError {
-  return s != null && typeof s === 'object' && 'stderr' in s;
+export function isExecaError(s: unknown): s is ExecaError & {code?: string} {
+  return s != null && typeof s === 'object' && 'exitCode' in s;
 }

addons/shared/debounce.ts

@@ -9,6 +9,7 @@ export type DebouncedFunction<Args extends Array<unknown>> = {
   (...args: Args): void;
   reset: () => void;
   isPending: () => boolean;
+  dispose: () => void;
 };
 
 /**
@@ -52,6 +53,7 @@ export function debounce<Args extends Array<unknown>>(
     if (leading) {
       callback = function () {
         shouldCallLeading = true;
+        clearTimeout(timeout);
         timeout = undefined;
       };
 
@@ -66,11 +68,13 @@ export function debounce<Args extends Array<unknown>>(
     } else {
       debouncer.reset();
       callback = function () {
+        clearTimeout(timeout);
         timeout = undefined;
         func.apply(context, args);
       };
     }
 
+    clearTimeout(timeout);
     timeout = setTimeout(callback, wait);
   }
 
@@ -80,6 +84,11 @@ export function debounce<Args extends Array<unknown>>(
     shouldCallLeading = true;
   };
 
+  debouncer.dispose = function () {
+    clearTimeout(timeout);
+    timeout = undefined;
+  };
+
   debouncer.isPending = function () {
     return timeout != null;
   };

addons/verify-addons-folder.py

@@ -45,8 +45,9 @@ async def verify(*, use_vendored_grammars=False):
         verify_shared(),
         verify_textmate(),
         verify_isl(),
-        verify_reviewstack(use_vendored_grammars=use_vendored_grammars),
     )
+    # shared depends on reviewstack generated textmate grammars, so can't run concurrently without flakiness
+    await verify_reviewstack(use_vendored_grammars=use_vendored_grammars)
 
 
 async def verify_prettier():