diff --git a/.github/workflows/sapling-addons.yml b/.github/workflows/sapling-addons.yml index 1d42972100c95..aefea6b688e35 100644 --- a/.github/workflows/sapling-addons.yml +++ b/.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 diff --git a/addons/isl-server/src/Repository.ts b/addons/isl-server/src/Repository.ts index ca385a1ad7ad4..8ad53e3f662f8 100644 --- a/addons/isl-server/src/Repository.ts +++ b/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, }; diff --git a/addons/isl-server/src/__tests__/analytics.test.ts b/addons/isl-server/src/__tests__/analytics.test.ts index 4eebf33398d57..3d5a12b8bd3df 100644 --- a/addons/isl-server/src/__tests__/analytics.test.ts +++ b/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) => { + 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', () => { diff --git a/addons/isl-server/src/github/githubCodeReviewProvider.ts b/addons/isl-server/src/github/githubCodeReviewProvider.ts index 26e7326c8a1ad..62aec5361355c 100644 --- a/addons/isl-server/src/github/githubCodeReviewProvider.ts +++ b/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 { diff --git a/addons/isl-server/src/github/queryGraphQL.ts b/addons/isl-server/src/github/queryGraphQL.ts index cd1f666448d34..679d41557c938 100644 --- a/addons/isl-server/src/github/queryGraphQL.ts +++ b/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( @@ -38,8 +36,17 @@ export default async function queryGraphQL( 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}`); @@ -47,16 +54,9 @@ export default async function queryGraphQL( // `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; } /** diff --git a/addons/isl-server/src/utils.ts b/addons/isl-server/src/utils.ts index 6e9c0793e4f59..31586ceb39560 100644 --- a/addons/isl-server/src/utils.ts +++ b/addons/isl-server/src/utils.ts @@ -165,6 +165,6 @@ export function parseExecJson( }); } -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; } diff --git a/addons/shared/debounce.ts b/addons/shared/debounce.ts index 8743fc995aeff..4bed539b1a771 100644 --- a/addons/shared/debounce.ts +++ b/addons/shared/debounce.ts @@ -9,6 +9,7 @@ export type DebouncedFunction> = { (...args: Args): void; reset: () => void; isPending: () => boolean; + dispose: () => void; }; /** @@ -52,6 +53,7 @@ export function debounce>( if (leading) { callback = function () { shouldCallLeading = true; + clearTimeout(timeout); timeout = undefined; }; @@ -66,11 +68,13 @@ export function debounce>( } 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>( shouldCallLeading = true; }; + debouncer.dispose = function () { + clearTimeout(timeout); + timeout = undefined; + }; + debouncer.isPending = function () { return timeout != null; }; diff --git a/addons/verify-addons-folder.py b/addons/verify-addons-folder.py index 0faa1a9cdfe7b..7798fb2e0aa81 100755 --- a/addons/verify-addons-folder.py +++ b/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():