From 4ecac022457b8bd50547e8e65d7092eb769e1bce Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 11 Dec 2024 17:17:51 -0500 Subject: [PATCH] chore(e2e-tests): consistently wait for successful exits of shell Do not use unqualified `shell.waitForExit()` in cases where we do not validate the exit code or check that no errors are displayed in the output, just assuming that the shell exited successfully, but instead intentionally always validate that these conditions are true in the cases in which we expect them to be. --- packages/e2e-tests/test/e2e-auth.spec.ts | 6 +- packages/e2e-tests/test/e2e-direct.spec.ts | 2 +- packages/e2e-tests/test/e2e-oidc.spec.ts | 10 +-- packages/e2e-tests/test/e2e-proxy.spec.ts | 4 +- packages/e2e-tests/test/e2e-snippet.spec.ts | 2 +- packages/e2e-tests/test/e2e-tls.spec.ts | 8 +- packages/e2e-tests/test/e2e.spec.ts | 74 ++++++++----------- packages/e2e-tests/test/test-shell-context.ts | 2 +- packages/e2e-tests/test/test-shell.ts | 18 +++-- 9 files changed, 59 insertions(+), 67 deletions(-) diff --git a/packages/e2e-tests/test/e2e-auth.spec.ts b/packages/e2e-tests/test/e2e-auth.spec.ts index 70c7bc83b..87e2ca9e4 100644 --- a/packages/e2e-tests/test/e2e-auth.spec.ts +++ b/packages/e2e-tests/test/e2e-auth.spec.ts @@ -998,7 +998,7 @@ describe('Auth e2e', function () { ], }); if ( - (await preTestShell.waitForExit()) === 1 && + (await preTestShell.waitForAnyExit()) === 1 && preTestShell.output.match( /digital envelope routines::unsupported|SSL routines::library has no ciphers/ ) @@ -1022,7 +1022,7 @@ describe('Auth e2e', function () { 'SCRAM-SHA-1', ], }); - await shell.waitForExit(); + await shell.waitForAnyExit(); try { shell.assertContainsOutput( 'Auth mechanism SCRAM-SHA-1 is not supported in FIPS mode' @@ -1109,7 +1109,7 @@ describe('Auth e2e', function () { 'GSSAPI', ], }); - await shell.waitForExit(); + await shell.waitForAnyExit(); // Failing to auth with kerberos fails with different error messages on each OS. // Sometimes in CI, it also fails because the server received kerberos // credentials, most likely because of a successful login by another diff --git a/packages/e2e-tests/test/e2e-direct.spec.ts b/packages/e2e-tests/test/e2e-direct.spec.ts index 913cdfc7c..46a030660 100644 --- a/packages/e2e-tests/test/e2e-direct.spec.ts +++ b/packages/e2e-tests/test/e2e-direct.spec.ts @@ -344,7 +344,7 @@ describe('e2e direct connection', function () { const shell = this.startTestShell({ args: ['--host', hostlist, 'admin'], }); - await shell.waitForExit(); + await shell.waitForAnyExit(); shell.assertContainsOutput('MongoServerSelectionError'); }); diff --git a/packages/e2e-tests/test/e2e-oidc.spec.ts b/packages/e2e-tests/test/e2e-oidc.spec.ts index 50d2f5582..9c6c16b20 100644 --- a/packages/e2e-tests/test/e2e-oidc.spec.ts +++ b/packages/e2e-tests/test/e2e-oidc.spec.ts @@ -249,7 +249,7 @@ describe('OIDC auth e2e', function () { '--browser=false', ], }); - await shell.waitForExit(); + await shell.waitForAnyExit(); shell.assertContainsOutput( 'Consider specifying --oidcFlows=auth-code,device-auth if you are running mongosh in an environment without browser access' ); @@ -405,7 +405,7 @@ describe('OIDC auth e2e', function () { MONGOSH_E2E_TEST_CURL_ALLOW_INVALID_TLS: '1', }, }); - await shell.waitForExit(); + await shell.waitForAnyExit(); // We cannot make the mongod server accept the mock IdP's certificate, // so the best we can verify here is that auth failed *on the server* shell.assertContainsOutput(/MongoServerError: Authentication failed/); @@ -435,7 +435,7 @@ describe('OIDC auth e2e', function () { }, }); - await shell.waitForExit(); + await shell.waitForAnyExit(); // We cannot make the mongod server accept the mock IdP's certificate, // so the best we can verify here is that auth failed *on the server* shell.assertContainsOutput(/MongoServerError: Authentication failed/); @@ -499,7 +499,7 @@ describe('OIDC auth e2e', function () { '--eval=42', ], }); - await shell.waitForExit(); + await shell.waitForSuccessfulExit(); shell.assertContainsOutput('BEGIN OIDC TOKEN DUMP'); shell.assertContainsOutput('"tokenType": "Bearer"'); @@ -519,7 +519,7 @@ describe('OIDC auth e2e', function () { '--eval=42', ], }); - await shell.waitForExit(); + await shell.waitForSuccessfulExit(); shell.assertContainsOutput('BEGIN OIDC TOKEN DUMP'); shell.assertContainsOutput('"tokenType": "Bearer"'); diff --git a/packages/e2e-tests/test/e2e-proxy.spec.ts b/packages/e2e-tests/test/e2e-proxy.spec.ts index b53a942ea..b9afc836d 100644 --- a/packages/e2e-tests/test/e2e-proxy.spec.ts +++ b/packages/e2e-tests/test/e2e-proxy.spec.ts @@ -271,7 +271,7 @@ describe('e2e proxy support', function () { }, }); - const code = await shell.waitForExit(); + const code = await shell.waitForAnyExit(); expect(code).to.equal(1); }); @@ -622,7 +622,7 @@ describe('e2e proxy support', function () { }, }); - await shell.waitForExit(); + await shell.waitForAnyExit(); // We cannot make the mongod server accept the mock IdP's certificate, // so the best we can verify here is that auth failed *on the server* shell.assertContainsOutput(/MongoServerError: Authentication failed/); diff --git a/packages/e2e-tests/test/e2e-snippet.spec.ts b/packages/e2e-tests/test/e2e-snippet.spec.ts index 70b3c1914..239e81862 100644 --- a/packages/e2e-tests/test/e2e-snippet.spec.ts +++ b/packages/e2e-tests/test/e2e-snippet.spec.ts @@ -87,7 +87,7 @@ describe('snippet integration tests', function () { 'config.set("snippetIndexSourceURLs", "http://localhost:1/")' ); shell.writeInputLine('exit'); - await shell.waitForExit(); + await shell.waitForSuccessfulExit(); shell = makeTestShell(); await shell.waitForPrompt(); diff --git a/packages/e2e-tests/test/e2e-tls.spec.ts b/packages/e2e-tests/test/e2e-tls.spec.ts index 7abe9c927..9d9c53a6e 100644 --- a/packages/e2e-tests/test/e2e-tls.spec.ts +++ b/packages/e2e-tests/test/e2e-tls.spec.ts @@ -92,8 +92,8 @@ describe('e2e TLS', function () { }); await shell.waitForPrompt(); await shell.executeLine('db.shutdownServer({ force: true })'); - shell.kill(); - await shell.waitForExit(); + shell.writeInputLine('exit'); + await shell.waitForAnyExit(); // closing the server may lead to an error being displayed }); const server = startTestServer('e2e-tls-no-cli-valid-srv', { @@ -399,8 +399,8 @@ describe('e2e TLS', function () { }); await shell.waitForPrompt(); await shell.executeLine('db.shutdownServer({ force: true })'); - shell.kill(); - await shell.waitForExit(); + shell.writeInputLine('exit'); + await shell.waitForAnyExit(); // closing the server may lead to an error being displayed }); const server = startTestServer('e2e-tls-valid-cli-valid-srv', { diff --git a/packages/e2e-tests/test/e2e.spec.ts b/packages/e2e-tests/test/e2e.spec.ts index acf2388ec..9fda3fbf6 100644 --- a/packages/e2e-tests/test/e2e.spec.ts +++ b/packages/e2e-tests/test/e2e.spec.ts @@ -35,9 +35,8 @@ describe('e2e', function () { describe('--version', function () { it('shows version', async function () { const shell = this.startTestShell({ args: ['--version'] }); - await shell.waitForExit(); + await shell.waitForSuccessfulExit(); - shell.assertNoErrors(); shell.assertContainsOutput(require('../package.json').version); }); }); @@ -45,9 +44,8 @@ describe('e2e', function () { describe('--build-info', function () { it('shows build info in JSON format', async function () { const shell = this.startTestShell({ args: ['--build-info'] }); - await shell.waitForExit(); + await shell.waitForSuccessfulExit(); - shell.assertNoErrors(); const data = JSON.parse(shell.output); expect(Object.keys(data)).to.deep.equal([ 'version', @@ -101,7 +99,7 @@ describe('e2e', function () { 'process.report.getReport()', ], }); - await shell.waitForExit(); + await shell.waitForSuccessfulExit(); processReport = JSON.parse(shell.output); } expect(data.runtimeGlibcVersion).to.equal( @@ -118,8 +116,7 @@ describe('e2e', function () { '--nodb', ], }); - await shell.waitForExit(); - shell.assertNoErrors(); + await shell.waitForSuccessfulExit(); const deps = JSON.parse(shell.output); expect(deps.nodeDriverVersion).to.be.a('string'); expect(deps.libmongocryptVersion).to.be.a('string'); @@ -176,34 +173,28 @@ describe('e2e', function () { }); }); it('closes the shell when "exit" is entered', async function () { - const onExit = shell.waitForExit(); shell.writeInputLine('exit'); - expect(await onExit).to.equal(0); + await shell.waitForSuccessfulExit(); }); it('closes the shell when "quit" is entered', async function () { - const onExit = shell.waitForExit(); shell.writeInputLine('quit'); - expect(await onExit).to.equal(0); + await shell.waitForSuccessfulExit(); }); it('closes the shell with the specified exit code when "exit(n)" is entered', async function () { - const onExit = shell.waitForExit(); shell.writeInputLine('exit(42)'); - expect(await onExit).to.equal(42); + expect(await shell.waitForAnyExit()).to.equal(42); }); it('closes the shell with the specified exit code when "quit(n)" is entered', async function () { - const onExit = shell.waitForExit(); shell.writeInputLine('quit(42)'); - expect(await onExit).to.equal(42); + expect(await shell.waitForAnyExit()).to.equal(42); }); it('closes the shell with the pre-specified exit code when "exit" is entered', async function () { - const onExit = shell.waitForExit(); shell.writeInputLine('process.exitCode = 42; exit()'); - expect(await onExit).to.equal(42); + expect(await shell.waitForAnyExit()).to.equal(42); }); it('closes the shell with the pre-specified exit code when "quit" is entered', async function () { - const onExit = shell.waitForExit(); shell.writeInputLine('process.exitCode = 42; quit()'); - expect(await onExit).to.equal(42); + expect(await shell.waitForAnyExit()).to.equal(42); }); it('decorates internal errors with bug reporting information', async function () { const err = await shell.executeLine( @@ -300,10 +291,8 @@ describe('e2e', function () { 'sleep(100);print([1,2,3,4,5,6,7,8,9,10].reduce(\n(a,b) => { return a*b; }, 1))\n\n\n\n', { end: true } ); - const exitCode = await shell.waitForExit(); - expect(exitCode).to.equal(0); + await shell.waitForSuccessfulExit(); shell.assertContainsOutput('3628800'); - shell.assertNoErrors(); }); }); @@ -1095,9 +1084,8 @@ describe('e2e', function () { '[db.hello()].reduce(\n() => { return 11111*11111; },0)\n\n\n', { end: true } ); - await shell.waitForExit(); + await shell.waitForSuccessfulExit(); shell.assertContainsOutput('123454321'); - shell.assertNoErrors(); }); }); @@ -1162,8 +1150,7 @@ describe('e2e', function () { // when run under coverage, as we currently specify the location of // coverage files via a relative path and nyc fails to write to that // when started from a changed cwd. - await shell.waitForExit(); - shell.assertNoErrors(); + await shell.waitForSuccessfulExit(); }); if (!jsContextFlags.includes('--jsContext=plain-vm')) { @@ -1202,7 +1189,7 @@ describe('e2e', function () { await eventually(() => { shell.assertContainsOutput('Error: uh oh'); }); - expect(await shell.waitForExit()).to.equal(1); + expect(await shell.waitForAnyExit()).to.equal(1); }); } }); @@ -1216,8 +1203,7 @@ describe('e2e', function () { await eventually(() => { shell.assertContainsOutput('hello one'); }); - expect(await shell.waitForExit()).to.equal(0); - shell.assertNoErrors(); + await shell.waitForSuccessfulExit(); }); if (!jsContextFlags.includes('--jsContext=plain-vm')) { @@ -1244,7 +1230,7 @@ describe('e2e', function () { await eventually(() => { shell.assertContainsOutput('Error: uh oh'); }); - expect(await shell.waitForExit()).to.equal(1); + expect(await shell.waitForAnyExit()).to.equal(1); }); it('fails with the error if the loaded script throws asynchronously (setImmediate)', async function () { @@ -1259,7 +1245,7 @@ describe('e2e', function () { await eventually(() => { shell.assertContainsOutput('Error: uh oh'); }); - expect(await shell.waitForExit()).to.equal( + expect(await shell.waitForAnyExit()).to.equal( jsContextFlags.includes('--jsContext=repl') ? 0 : 1 ); }); @@ -1276,7 +1262,7 @@ describe('e2e', function () { await eventually(() => { shell.assertContainsOutput('Error: uh oh'); }); - expect(await shell.waitForExit()).to.equal( + expect(await shell.waitForAnyExit()).to.equal( jsContextFlags.includes('--jsContext=repl') ? 0 : 1 ); }); @@ -1299,7 +1285,7 @@ describe('e2e', function () { script, ], }); - expect(await shell.waitForExit()).to.equal(0); + await shell.waitForSuccessfulExit(); // Check that: // - the script runs in the expected environment @@ -1512,7 +1498,7 @@ describe('e2e', function () { } await shell.executeLine('a = 42'); shell.writeInput('.exit\n'); - await shell.waitForExit(); + await shell.waitForSuccessfulExit(); shell = await startTestShell(); // Arrow up twice to skip the .exit line @@ -1521,7 +1507,7 @@ describe('e2e', function () { expect(shell.output).to.include('a = 42'); }); shell.writeInput('\n.exit\n'); - await shell.waitForExit(); + await shell.waitForSuccessfulExit(); expect(await fs.readFile(historyPath, 'utf8')).to.match(/^a = 42$/m); }); @@ -1533,7 +1519,7 @@ describe('e2e', function () { await shell.executeLine('a = 42'); shell.writeInput('.exit\n'); - await shell.waitForExit(); + await shell.waitForSuccessfulExit(); expect((await fs.stat(historyPath)).mode & 0o077).to.equal(0); }); @@ -1544,7 +1530,7 @@ describe('e2e', function () { await shell.executeLine('a = 42'); await shell.executeLine('foo = "bar"'); shell.writeInput('.exit\n'); - await shell.waitForExit(); + await shell.waitForAnyExit(); // db.auth() call fails because of --nodb const contents = await fs.readFile(historyPath, 'utf8'); expect(contents).to.not.match(/mypassword/); @@ -1603,7 +1589,7 @@ describe('e2e', function () { `config.set("updateURL", ${JSON.stringify(httpServerUrl)})` ); shell.writeInputLine('exit'); - await shell.waitForExit(); + await shell.waitForSuccessfulExit(); } delete env.CI; @@ -1623,13 +1609,13 @@ describe('e2e', function () { ).to.be.a('string'); }); shell.writeInputLine('exit'); - await shell.waitForExit(); + await shell.waitForSuccessfulExit(); } { const shell = await startTestShell(); shell.writeInputLine('exit'); - await shell.waitForExit(); + await shell.waitForSuccessfulExit(); shell.assertContainsOutput( 'mongosh 2023.4.15 is available for download: https://www.mongodb.com/try/download/shell' ); @@ -1799,7 +1785,7 @@ describe('e2e', function () { '.mongodb.net/', ], }); - const exitCode = await shell.waitForExit(); + const exitCode = await shell.waitForAnyExit(); expect(exitCode).to.equal(1); }); @@ -1813,7 +1799,7 @@ describe('e2e', function () { '.mongodb.net/', ], }); - const exitCode = await shell.waitForExit(); + const exitCode = await shell.waitForAnyExit(); expect(exitCode).to.equal(1); }); @@ -1904,7 +1890,7 @@ describe('e2e', function () { ); shell.writeInputLine('exit'); - await shell.waitForExit(); + await shell.waitForSuccessfulExit(); shell.assertNoErrors(); }); }); @@ -1937,7 +1923,7 @@ describe('e2e', function () { args: [filename], env: { ...process.env, MONGOSH_RUN_NODE_SCRIPT: '1' }, }); - expect(await shell.waitForExit()).to.equal(0); + await shell.waitForSuccessfulExit(); shell.assertContainsOutput('610'); }); }); diff --git a/packages/e2e-tests/test/test-shell-context.ts b/packages/e2e-tests/test/test-shell-context.ts index 583cd6b17..3241f68b1 100644 --- a/packages/e2e-tests/test/test-shell-context.ts +++ b/packages/e2e-tests/test/test-shell-context.ts @@ -60,7 +60,7 @@ export function ensureTestShellAfterHook( if (shell.process.exitCode === null) { shell.kill(); } - return shell.waitForExit(); + return shell.waitForAnyExit(); }) ); }); diff --git a/packages/e2e-tests/test/test-shell.ts b/packages/e2e-tests/test/test-shell.ts index 75e6f23cb..aa8744eb9 100644 --- a/packages/e2e-tests/test/test-shell.ts +++ b/packages/e2e-tests/test/test-shell.ts @@ -50,7 +50,7 @@ export class TestShell { /** * Starts a test shell. * - * Beware that the caller is responsible for calling {@link kill} (and potentially {@link waitForExit}). + * Beware that the caller is responsible for calling {@link kill} (and potentially {@link waitForAnyExit}). * * Consider calling the `startTestShell` function on a {@link Mocha.Context} instead, as that manages the lifetime the shell * and ensures it gets killed eventually. @@ -101,7 +101,7 @@ export class TestShell { const shell = new TestShell(shellProcess, options.consumeStdio); TestShell._openShells.add(shell); - void shell.waitForExit().then(() => { + void shell.waitForAnyExit().then(() => { TestShell._openShells.delete(shell); }); @@ -241,16 +241,20 @@ export class TestShell { ); } - waitForExit(): Promise { + waitForAnyExit(): Promise { return this._onClose; } + async waitForSuccessfulExit(): Promise { + assert.strictEqual(await this.waitForAnyExit(), 0); + this.assertNoErrors(); + } + /** * Waits for the shell to exit, asserts no errors and returns the output. */ async waitForCleanOutput(): Promise { - await this.waitForExit(); - this.assertNoErrors(); + await this.waitForSuccessfulExit(); return this.output; } @@ -261,7 +265,9 @@ export class TestShell { this.waitForPrompt(opts.start ?? 0, opts).then( () => ({ state: 'prompt' } as const) ), - this.waitForExit().then((c) => ({ state: 'exit', exitCode: c } as const)), + this.waitForAnyExit().then( + (c) => ({ state: 'exit', exitCode: c } as const) + ), ]); }