From 91fc6a085dc81fb27a08b7100d244d1a56d3d3e9 Mon Sep 17 00:00:00 2001 From: gagik Date: Thu, 6 Feb 2025 22:26:32 +0100 Subject: [PATCH 1/7] wip: check what path sep is when using windows runner --- packages/types/src/index.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index bdf495f93a..42cfa5b601 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -588,6 +588,13 @@ export class CliUserConfigValidator extends SnippetShellUserConfigValidator { value !== undefined && (typeof value !== 'string' || !path.isAbsolute(value)) ) { + // eslint-disable-next-line no-console + console.info( + path.sep, + path.win32 === path, + path.isAbsolute(value as string), + value + ); return `${key} must be a valid absolute path or empty`; } return null; From 8420042c06e6111d322020d1539cea64fffb4026 Mon Sep 17 00:00:00 2001 From: gagik Date: Fri, 7 Feb 2025 11:58:51 +0100 Subject: [PATCH 2/7] Revert "wip: check what path sep is when using windows runner" This reverts commit 85453b9bf32bbdd3d02f62b90ece16559749f7fb. --- packages/types/src/index.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 42cfa5b601..bdf495f93a 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -588,13 +588,6 @@ export class CliUserConfigValidator extends SnippetShellUserConfigValidator { value !== undefined && (typeof value !== 'string' || !path.isAbsolute(value)) ) { - // eslint-disable-next-line no-console - console.info( - path.sep, - path.win32 === path, - path.isAbsolute(value as string), - value - ); return `${key} must be a valid absolute path or empty`; } return null; From cc82c07ca1bb0ee036ca9fdee1ff46350f81f18d Mon Sep 17 00:00:00 2001 From: gagik Date: Tue, 4 Feb 2025 13:27:03 +0100 Subject: [PATCH 3/7] feat(logging): add max file count configuration MONGOSH-1987 --- packages/cli-repl/src/cli-repl.spec.ts | 14 ++++++++++++++ packages/cli-repl/src/cli-repl.ts | 3 ++- packages/types/src/index.spec.ts | 6 ++++++ packages/types/src/index.ts | 2 ++ 4 files changed, 24 insertions(+), 1 deletion(-) diff --git a/packages/cli-repl/src/cli-repl.spec.ts b/packages/cli-repl/src/cli-repl.spec.ts index b9af6f9bd4..a6ca73ec1e 100644 --- a/packages/cli-repl/src/cli-repl.spec.ts +++ b/packages/cli-repl/src/cli-repl.spec.ts @@ -324,6 +324,7 @@ describe('CliRepl', function () { 'disableLogging', 'logLocation', 'logRetentionDays', + 'logMaxFileCount', ] satisfies (keyof CliUserConfig)[]); }); @@ -1457,6 +1458,19 @@ describe('CliRepl', function () { testRetentionDays ); }); + + it('can set log max file count', async function () { + const testMaxFileCount = 123; + cliRepl.config.logMaxFileCount = testMaxFileCount; + await cliRepl.start(await testServer.connectionString(), {}); + + expect(cliRepl.getConfig('logMaxFileCount')).equals( + testMaxFileCount + ); + expect(cliRepl.logManager?._options.maxLogFileCount).equals( + testMaxFileCount + ); + }); }); it('times out fast', async function () { diff --git a/packages/cli-repl/src/cli-repl.ts b/packages/cli-repl/src/cli-repl.ts index 88e05a4284..2cb3214d99 100644 --- a/packages/cli-repl/src/cli-repl.ts +++ b/packages/cli-repl/src/cli-repl.ts @@ -261,7 +261,8 @@ export class CliRepl implements MongoshIOProvider { this.shellHomeDirectory.localPath('.'), retentionDays: await this.getConfig('logRetentionDays'), maxLogFileCount: +( - process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT || 100 + process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT || + (await this.getConfig('logMaxFileCount')) ), onerror: (err: Error) => this.bus.emit('mongosh:error', err, 'log'), onwarn: (err: Error, path: string) => diff --git a/packages/types/src/index.spec.ts b/packages/types/src/index.spec.ts index 1393129358..7d1322f1b6 100644 --- a/packages/types/src/index.spec.ts +++ b/packages/types/src/index.spec.ts @@ -31,6 +31,12 @@ describe('config validation', function () { expect(await validate('logRetentionDays', -1)).to.equal( 'logRetentionDays must be a positive integer' ); + expect(await validate('logMaxFileCount', 'foo')).to.equal( + 'logMaxFileCount must be a positive integer' + ); + expect(await validate('logMaxFileCount', -1)).to.equal( + 'logMaxFileCount must be a positive integer' + ); expect(await validate('showStackTraces', 'foo')).to.equal( 'showStackTraces must be a boolean' ); diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index bdf495f93a..ab8134bdb7 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -510,6 +510,7 @@ export class CliUserConfig extends SnippetShellUserConfig { disableLogging = false; logLocation: string | undefined = undefined; logRetentionDays = 30; + logMaxFileCount = 100; } export class CliUserConfigValidator extends SnippetShellUserConfigValidator { @@ -533,6 +534,7 @@ export class CliUserConfigValidator extends SnippetShellUserConfigValidator { case 'inspectDepth': case 'historyLength': case 'logRetentionDays': + case 'logMaxFileCount': if (typeof value !== 'number' || value < 0) { return `${key} must be a positive integer`; } From 6d8a79483ebba15cdce0a7889ec11d58aa4bf5e8 Mon Sep 17 00:00:00 2001 From: gagik Date: Fri, 7 Feb 2025 14:53:57 +0100 Subject: [PATCH 4/7] tests: add end to end deletion test --- packages/e2e-tests/test/e2e.spec.ts | 52 +++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/packages/e2e-tests/test/e2e.spec.ts b/packages/e2e-tests/test/e2e.spec.ts index 93c8cc373f..dceb988c23 100644 --- a/packages/e2e-tests/test/e2e.spec.ts +++ b/packages/e2e-tests/test/e2e.spec.ts @@ -1747,6 +1747,58 @@ describe('e2e', function () { }); }); + describe('with custom log retention max file count', function () { + const customLogDir = useTmpdir(); + + it('should delete files once it is above the max file count limit', async function () { + const globalConfig = path.join(homedir, 'globalconfig.conf'); + await fs.writeFile( + globalConfig, + `mongosh:\n logLocation: ${JSON.stringify( + customLogDir.path + )}\n logMaxFileCount: 4` + ); + const paths: string[] = []; + const offset = Math.floor(Date.now() / 1000); + + // Create 10 log files + for (let i = 9; i >= 0; i--) { + const filename = path.join( + customLogDir.path, + ObjectId.createFromTime(offset - i).toHexString() + '_log' + ); + await fs.writeFile(filename, ''); + paths.push(filename); + } + + // All 10 existing log files exist. + expect(await getFilesState(paths)).to.equal('1111111111'); + shell = this.startTestShell({ + args: ['--nodb'], + env: { + ...env, + MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT: '', + MONGOSH_GLOBAL_CONFIG_FILE_FOR_TESTING: globalConfig, + }, + forceTerminal: true, + }); + + await shell.waitForPrompt(); + + // Add the newly created log to the file list. + paths.push( + path.join(customLogDir.path, `${shell.logId as string}_log`) + ); + + expect( + await shell.executeLine('config.get("logMaxFileCount")') + ).contains('4'); + + // Expect 7 files to be deleted and 4 to remain (including the new log file) + expect(await getFilesState(paths)).to.equal('00000001111'); + }); + }); + it('creates a log file that keeps track of session events', async function () { expect(await shell.executeLine('print(123 + 456)')).to.include('579'); const log = await readLogFile(); From a265439557148282b99b566cf9dcc73356309206 Mon Sep 17 00:00:00 2001 From: gagik Date: Mon, 10 Feb 2025 09:55:45 +0100 Subject: [PATCH 5/7] fix(tests): use await on getting config --- packages/cli-repl/src/cli-repl.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli-repl/src/cli-repl.spec.ts b/packages/cli-repl/src/cli-repl.spec.ts index a6ca73ec1e..ad4d64fdff 100644 --- a/packages/cli-repl/src/cli-repl.spec.ts +++ b/packages/cli-repl/src/cli-repl.spec.ts @@ -1464,7 +1464,7 @@ describe('CliRepl', function () { cliRepl.config.logMaxFileCount = testMaxFileCount; await cliRepl.start(await testServer.connectionString(), {}); - expect(cliRepl.getConfig('logMaxFileCount')).equals( + expect(await cliRepl.getConfig('logMaxFileCount')).equals( testMaxFileCount ); expect(cliRepl.logManager?._options.maxLogFileCount).equals( From 14e1a127ed9006a11e2af202aacc9cf816132f87 Mon Sep 17 00:00:00 2001 From: gagik Date: Mon, 10 Feb 2025 11:57:56 +0100 Subject: [PATCH 6/7] fix(tests): unset MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT before running tests --- packages/cli-repl/src/cli-repl.spec.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/cli-repl/src/cli-repl.spec.ts b/packages/cli-repl/src/cli-repl.spec.ts index ad4d64fdff..054bfed341 100644 --- a/packages/cli-repl/src/cli-repl.spec.ts +++ b/packages/cli-repl/src/cli-repl.spec.ts @@ -1462,6 +1462,9 @@ describe('CliRepl', function () { it('can set log max file count', async function () { const testMaxFileCount = 123; cliRepl.config.logMaxFileCount = testMaxFileCount; + const oldEnvironmentLimit = + process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT; + delete process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT; await cliRepl.start(await testServer.connectionString(), {}); expect(await cliRepl.getConfig('logMaxFileCount')).equals( @@ -1470,6 +1473,9 @@ describe('CliRepl', function () { expect(cliRepl.logManager?._options.maxLogFileCount).equals( testMaxFileCount ); + + process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT = + oldEnvironmentLimit; }); }); From 38e7ec2c1d1781726c5089190f006b10c27be557 Mon Sep 17 00:00:00 2001 From: gagik Date: Mon, 10 Feb 2025 13:11:55 +0100 Subject: [PATCH 7/7] fix: minimize flakiness of a log test --- packages/e2e-tests/test/e2e.spec.ts | 35 ++++++++++++++++------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/packages/e2e-tests/test/e2e.spec.ts b/packages/e2e-tests/test/e2e.spec.ts index dceb988c23..5bb38a1d56 100644 --- a/packages/e2e-tests/test/e2e.spec.ts +++ b/packages/e2e-tests/test/e2e.spec.ts @@ -1868,23 +1868,26 @@ describe('e2e', function () { const newLogId = shell.logId; expect(newLogId).not.null; expect(oldLogId).equals(newLogId); - log = await readLogFile(); - expect( - log.filter( - (logEntry) => logEntry.attr?.input === 'print(111 + 222)' - ) - ).to.have.lengthOf(1); - expect( - log.filter( - (logEntry) => logEntry.attr?.input === 'print(579 - 123)' - ) - ).to.have.lengthOf(1); - expect( - log.filter( - (logEntry) => logEntry.attr?.input === 'print(123 + 456)' - ) - ).to.have.lengthOf(0); + await eventually(async () => { + log = await readLogFile(); + + expect( + log.filter( + (logEntry) => logEntry.attr?.input === 'print(111 + 222)' + ) + ).to.have.lengthOf(1); + expect( + log.filter( + (logEntry) => logEntry.attr?.input === 'print(579 - 123)' + ) + ).to.have.lengthOf(1); + expect( + log.filter( + (logEntry) => logEntry.attr?.input === 'print(123 + 456)' + ) + ).to.have.lengthOf(0); + }); }); it('includes information about the driver version', async function () {