reviewstack: fix verify addons/ folder github ci

analytics.test.ts was trying to call the real gh cli rather than a mock, which was causing errors when gh not installed, and process and handle leaks when gh was installed. Fixed bu using mock execa as we don't need real gh to test the analytics tracking

Jest was also reporting leaks of timeouts after above fix, so added dispose() to debounce.ts and call dispose() from githubCodeReviewProvider

Found shared/ depends on reviewstack generated textmate grammars, so can't run concurrently without flakiness (fails with missing import).  Fixed test ordering in verify-addons-folder.py

Tests also log about missing watchman which triggers a failure due to logs after test exit, even if watchman disabled. Install fb-watchman to silence it

Tes plan:

Before, ./verify-addons-folder.py tests broken in CI with:
```
Error: Unhandled error. (Error: GhNotInstalledError: Error: Command failed with ENOENT: gh api graphql -f searchQuery=repo:facebook/sapling is:pr author:@me -F numToFetch=100 --hostname github.com -f query=
    query YourPullRequestsQuery($searchQuery: String!, $numToFetch: Int!) {
  search(query: $searchQuery, type: ISSUE, first: $numToFetch) {
    nodes {
      ... on PullRequest {
```
```

After, works

.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 && dpkg -i watchman.deb; apt -y -f install
       - 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():