Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(tests): use logMaxFileCount instead of a environment variable in tests MONGOSH-2009 #2366

Merged
merged 1 commit into from
Feb 12, 2025
Merged

chore(tests): use logMaxFileCount instead of a environment variable in tests MONGOSH-2009 #2366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .evergreen/setup-env.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ fi
export BASEDIR="$PWD/.evergreen"
export PATH="/cygdrive/c/python/Python311/Scripts:/cygdrive/c/python/Python311:/cygdrive/c/Python311/Scripts:/cygdrive/c/Python311:/opt/python/3.6/bin:$BASEDIR/mingit/cmd:$BASEDIR/mingit/mingw64/libexec/git-core:$BASEDIR/git-2:$BASEDIR/npm-10/node_modules/.bin:$BASEDIR/node-v$NODE_JS_VERSION-win-x64:/opt/java/jdk16/bin:/opt/chefdk/gitbin:/cygdrive/c/cmake/bin:$TOOLCHAIN_PATH:$PATH"

export MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT=100000
export MONGOSH_GLOBAL_CONFIG_FILE_FOR_TESTING="$BASEDIR/../../testing/tests-globalconfig.conf"

export IS_MONGOSH_EVERGREEN_CI=1
export DEBUG="mongodb*,$DEBUG"

Expand Down
6 changes: 0 additions & 6 deletions packages/cli-repl/src/cli-repl.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1463,9 +1463,6 @@ 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(
Expand All @@ -1474,9 +1471,6 @@ describe('CliRepl', function () {
expect(cliRepl.logManager?._options.maxLogFileCount).equals(
testMaxFileCount
);

process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT =
oldEnvironmentLimit;
});

it('can set log compression', async function () {
Expand Down
5 changes: 1 addition & 4 deletions packages/cli-repl/src/cli-repl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,10 +261,7 @@ export class CliRepl implements MongoshIOProvider {
this.shellHomeDirectory.localPath('.'),
retentionDays: await this.getConfig('logRetentionDays'),
gzip: await this.getConfig('logCompressionEnabled'),
maxLogFileCount: +(
process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT ||
(await this.getConfig('logMaxFileCount'))
),
maxLogFileCount: await this.getConfig('logMaxFileCount'),
onerror: (err: Error) => this.bus.emit('mongosh:error', err, 'log'),
onwarn: (err: Error, path: string) =>
this.warnAboutInaccessibleFile(err, path),
Expand Down
51 changes: 17 additions & 34 deletions packages/e2e-tests/test/e2e.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1414,7 +1414,7 @@ describe('e2e', function () {
startTestShell = async (...extraArgs: string[]) => {
const shell = this.startTestShell({
args: ['--nodb', ...extraArgs],
env: env,
env,
forceTerminal: true,
});
await shell.waitForPrompt();
Expand Down Expand Up @@ -1467,10 +1467,8 @@ describe('e2e', function () {
);
shell = this.startTestShell({
args: ['--nodb'],
env: {
...env,
MONGOSH_GLOBAL_CONFIG_FILE_FOR_TESTING: globalConfig,
},
env,
globalConfigPath: globalConfig,
forceTerminal: true,
});
await shell.waitForPrompt();
Expand Down Expand Up @@ -1524,10 +1522,8 @@ describe('e2e', function () {
await fs.writeFile(globalConfig, 'mongosh:\n disableLogging: true');
shell = this.startTestShell({
args: ['--nodb'],
env: {
...env,
MONGOSH_GLOBAL_CONFIG_FILE_FOR_TESTING: globalConfig,
},
env,
globalConfigPath: globalConfig,
forceTerminal: true,
});
await shell.waitForPrompt();
Expand All @@ -1545,10 +1541,8 @@ describe('e2e', function () {
await fs.writeFile(globalConfig, 'mongosh:\n disableLogging: false');
shell = this.startTestShell({
args: ['--nodb'],
env: {
...env,
MONGOSH_GLOBAL_CONFIG_FILE_FOR_TESTING: globalConfig,
},
env,
globalConfigPath: globalConfig,
forceTerminal: true,
});
await shell.waitForPrompt();
Expand Down Expand Up @@ -1580,10 +1574,8 @@ describe('e2e', function () {

shell = this.startTestShell({
args: ['--nodb'],
env: {
...env,
MONGOSH_GLOBAL_CONFIG_FILE_FOR_TESTING: globalConfig,
},
env,
globalConfigPath: globalConfig,
forceTerminal: true,
});
await shell.waitForPrompt();
Expand All @@ -1610,10 +1602,8 @@ describe('e2e', function () {

shell = this.startTestShell({
args: ['--nodb'],
env: {
...env,
MONGOSH_GLOBAL_CONFIG_FILE_FOR_TESTING: globalConfig,
},
env,
globalConfigPath: globalConfig,
forceTerminal: true,
});
await shell.waitForPrompt();
Expand Down Expand Up @@ -1705,10 +1695,8 @@ describe('e2e', function () {

shell = this.startTestShell({
args: ['--nodb'],
env: {
...env,
MONGOSH_GLOBAL_CONFIG_FILE_FOR_TESTING: globalConfig,
},
env,
globalConfigPath: globalConfig,
forceTerminal: true,
});

Expand Down Expand Up @@ -1774,10 +1762,8 @@ describe('e2e', function () {

shell = this.startTestShell({
args: ['--nodb'],
env: {
...env,
MONGOSH_GLOBAL_CONFIG_FILE_FOR_TESTING: globalConfig,
},
env,
globalConfigPath: globalConfig,
forceTerminal: true,
});

Expand Down Expand Up @@ -1818,11 +1804,8 @@ describe('e2e', function () {
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,
},
env,
globalConfigPath: globalConfig,
forceTerminal: true,
});

Expand Down
8 changes: 8 additions & 0 deletions packages/e2e-tests/test/test-shell.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export interface TestShellOptions {
cwd?: string;
forceTerminal?: boolean;
consumeStdio?: boolean;
globalConfigPath?: string;
}

/**
Expand All @@ -63,6 +64,13 @@ export class TestShell {
env = { ...env, MONGOSH_FORCE_TERMINAL: '1' };
}

if (options.globalConfigPath) {
env = {
...env,
MONGOSH_GLOBAL_CONFIG_FILE_FOR_TESTING: options.globalConfigPath,
};
}

const args = [...options.args];
if (process.env.MONGOSH_TEST_E2E_FORCE_FIPS) {
args.push('--tlsFIPSMode');
Expand Down
3 changes: 3 additions & 0 deletions testing/tests-globalconfig.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Default global configuration used by tests
mongosh:
Copy link
Contributor Author

@gagik gagik Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is the best way to go about it (also will see how CI reacts...; seems good so far)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can create a file during testing, write to it, and then delete it as a test suit cleanup. Similarly how we do here: https://github.com/mongodb-js/mongosh/blob/main/packages/e2e-tests/test/e2e.spec.ts#L1463-L1467

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, or you could just do config.set(...) in one shell and then start another one, we also do that in a couple of places (has the advantage of not requiring any explicit disk I/O)

Copy link
Contributor Author

@gagik gagik Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my thinking was to make it consistent with everything that would otherwise be using the environment variable so at least in both cli-repl and e2e-tests which is why I thought to keep the actual settings being set in setup-env.sh instead of some package's test setup code.

Isn't it better to have this global file instead of the alternatives? It seems less restricted to any given package and avoids more computation of building a file or spawning shells for every test.

logMaxFileCount: 100000
Loading