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.

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

Conversation

gagik
Copy link
Contributor

@gagik gagik commented Feb 12, 2025

Previously we relied on MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT for setting max log files. Now we can do so by defining a global config for tests.

@gagik gagik changed the title chore(tests): use logMaxFileCount instead of a environment variable in tests MONGOSH-2009 chore(tests): use logMaxFileCount instead of a environment variable in tests MONGOSH-2009 Feb 12, 2025
@@ -0,0 +1,3 @@
# Default global configuration used by tests
mongosh:
Copy link
Contributor Author

@gagik gagik Feb 12, 2025

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

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.

@gagik gagik changed the base branch from gagik/use-prefix to main February 12, 2025 10:31
@gagik gagik changed the base branch from main to gagik/use-prefix February 12, 2025 10:32
@gagik gagik force-pushed the gagik/get-rid-of-env branch from f1ef7ff to dd32591 Compare February 12, 2025 10:33
@gagik gagik changed the base branch from gagik/use-prefix to main February 12, 2025 10:33
…FILE_COUNT environment variable MONGOSH-2009

Previously we relied on MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT for setting max log files. Now we can do so by defining a global config for tests.
@gagik gagik force-pushed the gagik/get-rid-of-env branch from dd32591 to 89719dc Compare February 12, 2025 10:34
@gagik gagik requested a review from alenakhineika February 12, 2025 11:16
@gagik gagik merged commit a5e517c into main Feb 12, 2025
123 of 127 checks passed
@gagik gagik deleted the gagik/get-rid-of-env branch February 12, 2025 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants