From 3a9e633c71038211fe4bffadccd4f7cf2f7f7da4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 18 Feb 2021 15:43:31 +0100 Subject: [PATCH 1/2] fix(cli-repl): defer exit event until evalutation done This otherwise messes with cases where the input stream ends while async evaluation is still in progress (e.g. when piping into mongosh). --- packages/cli-repl/src/async-repl.spec.ts | 20 ++++++++++++++++++++ packages/cli-repl/src/async-repl.ts | 20 ++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/packages/cli-repl/src/async-repl.spec.ts b/packages/cli-repl/src/async-repl.spec.ts index 6facb75d0a..e19f231f33 100644 --- a/packages/cli-repl/src/async-repl.spec.ts +++ b/packages/cli-repl/src/async-repl.spec.ts @@ -6,6 +6,7 @@ import { once } from 'events'; import chai, { expect } from 'chai'; import sinon from 'ts-sinon'; import sinonChai from 'sinon-chai'; +import { tick } from '../test/repl-helpers'; chai.use(sinonChai); const delay = promisify(setTimeout); @@ -123,6 +124,25 @@ describe('AsyncRepl', () => { expect(foundUid).to.be.true; }); + it('delays the "exit" event until after asynchronous evaluation is finished', async() => { + const { input, repl } = createDefaultAsyncRepl(); + let exited = false; + repl.on('exit', () => { exited = true; }); + + let resolve; + repl.context.asyncFn = () => new Promise((res) => { resolve = res; }); + + input.end('asyncFn()\n'); + expect(exited).to.be.false; + + await tick(); + resolve(); + expect(exited).to.be.false; + + await tick(); + expect(exited).to.be.true; + }); + describe('allows handling exceptions from e.g. the writer function', () => { it('for succesful completions', async() => { const error = new Error('throwme'); diff --git a/packages/cli-repl/src/async-repl.ts b/packages/cli-repl/src/async-repl.ts index 159c9a01e7..c6100a40ac 100644 --- a/packages/cli-repl/src/async-repl.ts +++ b/packages/cli-repl/src/async-repl.ts @@ -49,8 +49,13 @@ export function start(opts: AsyncREPLOptions): REPLServer { repl.emit(evalStart, { input } as EvalStartEvent); try { + let exitEventPending = false; + const exitListener = () => { exitEventPending = true; }; + let previousExitListeners: any[] = []; + let sigintListener: (() => void) | undefined = undefined; let previousSigintListeners: any[] = []; + try { result = await new Promise((resolve, reject) => { if (breakEvalOnSigint) { @@ -68,6 +73,13 @@ export function start(opts: AsyncREPLOptions): REPLServer { repl.once('SIGINT', sigintListener); } + // The REPL may become over-eager and emit 'exit' events while our + // evaluation is still in progress (because it doesn't expect async + // evaluation). If that happens, defer the event until later. + previousExitListeners = repl.rawListeners('exit'); + repl.removeAllListeners('exit'); + repl.once('exit', exitListener); + const evalResult = asyncEval(originalEval, input, context, filename); if (sigintListener !== undefined) { @@ -84,6 +96,14 @@ export function start(opts: AsyncREPLOptions): REPLServer { for (const listener of previousSigintListeners) { repl.on('SIGINT', listener); } + + repl.removeListener('exit', exitListener); + for (const listener of previousExitListeners) { + repl.on('exit', listener); + } + if (exitEventPending) { + process.nextTick(() => repl.emit('exit')); + } } } catch (err) { try { From cb0f339b3de892de2377e2d8b89e6e3ca7005f12 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 18 Feb 2021 15:46:37 +0100 Subject: [PATCH 2/2] chore(cli-repl): add Segment API integration test MONGOSH-363 Add a test that verifies the actual API key bundled into the actual release binary. In order to achieve this, add an internal test command that performs a Segment API request and verifies that its callback does not return an error. Note that this is, currently, a test that with somewhat limited meaningfullness, because the Segment API currently always gives successful HTTP responses, regardless of the validitity of the API key. --- packages/cli-repl/src/arg-parser.ts | 6 ++++-- packages/cli-repl/src/cli-repl.spec.ts | 9 +++++++++ packages/cli-repl/src/cli-repl.ts | 16 ++++++++++++++-- packages/cli-repl/src/mongosh-repl.ts | 6 +++++- packages/cli-repl/src/smoke-tests.ts | 8 +++++++- 5 files changed, 39 insertions(+), 6 deletions(-) diff --git a/packages/cli-repl/src/arg-parser.ts b/packages/cli-repl/src/arg-parser.ts index 3f64f4bd93..6798c76a45 100644 --- a/packages/cli-repl/src/arg-parser.ts +++ b/packages/cli-repl/src/arg-parser.ts @@ -45,6 +45,7 @@ const OPTIONS = { boolean: [ 'async', 'help', + 'internalTestCommands', 'ipv6', 'nodb', 'norc', @@ -87,6 +88,7 @@ function getLocale(args: string[], env: any): string { return lang ? lang.split('.')[0] : lang; } +type AllCliOptions = CliOptions & { smokeTests: boolean, internalTestCommands: boolean }; /** * Parses arguments into a JS object. * @@ -94,7 +96,7 @@ function getLocale(args: string[], env: any): string { * * @returns {CliOptions} The arguments as cli options. */ -function parse(args: string[]): (CliOptions & { smokeTests: boolean }) { +function parse(args: string[]): AllCliOptions { const programArgs = args.slice(2); i18n.setLocale(getLocale(programArgs, process.env)); @@ -111,7 +113,7 @@ function parse(args: string[]): (CliOptions & { smokeTests: boolean }) { ${USAGE}` ); }); - return parsed as unknown as (CliOptions & { smokeTests: boolean }); + return parsed as unknown as AllCliOptions; } export default parse; diff --git a/packages/cli-repl/src/cli-repl.spec.ts b/packages/cli-repl/src/cli-repl.spec.ts index d01b3aeb10..4e98cbf6b0 100644 --- a/packages/cli-repl/src/cli-repl.spec.ts +++ b/packages/cli-repl/src/cli-repl.spec.ts @@ -379,6 +379,7 @@ describe('CliRepl', () => { await once(srv, 'listening'); host = `http://localhost:${(srv.address() as any).port}`; cliReplOptions.analyticsOptions = { host, apiKey: '🔑' }; + cliReplOptions.shellCliOptions.internalTestCommands = true; cliRepl = new CliRepl(cliReplOptions); await cliRepl.start(await testServer.connectionString(), {}); }); @@ -417,6 +418,14 @@ describe('CliRepl', () => { req => JSON.parse(req.body).batch.filter(entry => entry.event === 'Use')).flat(); expect(useEvents).to.have.lengthOf(2); }); + + it('can self-test the analytics implementation', async() => { + input.write('__verifyAnalytics();\n'); + await waitEval(cliRepl.bus); + const selfTestEvents = requests.map( + req => JSON.parse(req.body).batch.filter(entry => entry.event === '__verifyAnalytics')).flat(); + expect(selfTestEvents).to.have.lengthOf(1); + }); }); context('without network connectivity', () => { diff --git a/packages/cli-repl/src/cli-repl.ts b/packages/cli-repl/src/cli-repl.ts index 22b8ea7d2e..7b2da97eb0 100644 --- a/packages/cli-repl/src/cli-repl.ts +++ b/packages/cli-repl/src/cli-repl.ts @@ -27,7 +27,7 @@ const CONNECTING = 'cli-repl.cli-repl.connecting'; type AnalyticsOptions = { host?: string, apiKey?: string }; export type CliReplOptions = { - shellCliOptions: CliOptions & { mongocryptdSpawnPath?: string }, + shellCliOptions: CliOptions & { mongocryptdSpawnPath?: string, internalTestCommands?: boolean }, input: Readable; output: Writable; shellHomePaths: ShellHomePaths; @@ -41,7 +41,7 @@ export type CliReplOptions = { class CliRepl { mongoshRepl: MongoshNodeRepl; bus: MongoshBus; - cliOptions: CliOptions; + cliOptions: CliOptions & { internalTestCommands?: boolean }; shellHomeDirectory: ShellHomeDirectory; configDirectory: ConfigManager; config: UserConfig = new UserConfig(); @@ -140,6 +140,18 @@ class CliRepl { const initialServiceProvider = await this.connect(driverUri, driverOptions); await this.mongoshRepl.start(initialServiceProvider); + + if (this.analytics && this.cliOptions.internalTestCommands) { + const analytics = this.analytics; + this.mongoshRepl.context.__verifyAnalytics = async() => { + const promise = promisify(analytics.track.bind(analytics))({ + userId: this.config.userId, event: '__verifyAnalytics' + }); + analytics.flush(); + await promise; + return 'API call succeeded'; + }; + } } /** diff --git a/packages/cli-repl/src/mongosh-repl.ts b/packages/cli-repl/src/mongosh-repl.ts index baa4750fb6..2d24f97bb7 100644 --- a/packages/cli-repl/src/mongosh-repl.ts +++ b/packages/cli-repl/src/mongosh-repl.ts @@ -230,7 +230,7 @@ class MongoshNodeRepl implements EvaluationListener { } catch { /* ... */ } }); - internalState.setCtx(repl.context); + internalState.setCtx(this.context); // Only start reading from the input *after* we set up everything, including // internalState.setCtx(). this.lineByLineInput.start(); @@ -387,6 +387,10 @@ class MongoshNodeRepl implements EvaluationListener { return this._runtimeState; } + get context(): any { + return this.runtimeState().repl.context; + } + async close(): Promise { const rs = this._runtimeState; if (rs) { diff --git a/packages/cli-repl/src/smoke-tests.ts b/packages/cli-repl/src/smoke-tests.ts index 0a8969e789..b1c4100172 100644 --- a/packages/cli-repl/src/smoke-tests.ts +++ b/packages/cli-repl/src/smoke-tests.ts @@ -14,6 +14,7 @@ export async function runSmokeTests(smokeTestServer: string | undefined, executa assert(!!smokeTestServer, 'Make sure MONGOSH_SMOKE_TEST_SERVER is set in CI'); } + let n = 0; for (const { input, output, testArgs } of [{ input: 'print("He" + "llo" + " Wor" + "ld!")', output: /Hello World!/, @@ -22,10 +23,15 @@ export async function runSmokeTests(smokeTestServer: string | undefined, executa input: fleSmokeTestScript, output: /Test succeeded|Test skipped/, testArgs: [smokeTestServer as string] + }] : []).concat(process.execPath === process.argv[1] ? [{ + input: '__verifyAnalytics()', + output: /API call succeeded/, + testArgs: ['--internalTestCommands', '--nodb'] }] : [])) { + n++; await runSmokeTest(executable, [...args, ...testArgs], input, output); } - console.log('all tests passed'); + console.log(`${n} tests passed!`); } async function runSmokeTest(executable: string, args: string[], input: string, output: RegExp): Promise {