-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
Update MongoDB.md #6412
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
Update MongoDB.md #6412
Conversation
The example was misleading because it would only work when running a single test or with the option `--runInBand`. This is a fix to get around the fact that all tests run in their own contexts.
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Mind running |
@SimenB thanks was looking for the linting command 👍 |
Codecov Report
@@ Coverage Diff @@
## master #6412 +/- ##
=======================================
Coverage 63.48% 63.48%
=======================================
Files 227 227
Lines 8697 8697
Branches 4 3 -1
=======================================
Hits 5521 5521
Misses 3175 3175
Partials 1 1 Continue to review full report at Codecov.
|
Could you copy the changes into the versioned 23.0 directory as well? |
@vladgolubev what do you think of the changes here? |
version: '3.2.19', | ||
}, | ||
}); | ||
const globalConfigPath = path.join(__dirname, 'globalConfig.json'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really necessary to create a file in project folder? I noticed such approach with jest-puppeteer example, but at least it creates file in /tmp
, and anyway, assigning to global
should work fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assigning to global
only works when tests are run in band, not across workers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we pass config
to the setup (and if not, we can start doing it) where you an lookup cacheDirectory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the little digging I did we don't have access to config in the setup, atleast not in an easy way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's #5957
@vladgolubev yeah I wasn’t too sure where to write the file for the example without it being confusing... I was thinking about it... do you guys reckon it would be too much of a performance hog to advocate spinning up a memory mongod directly in the MongoEnvironment? (Of course this would mean an entirely different DB for each tests, but thats good if you ask me). async setup() {
console.log('Setup MongoDB Test Environment');
this.global.__MONGOD__ = new MongodbMemoryServer();
this.global.__MONGO_URI__ = await this.global.__MONGOD__.getConnectionString();
//use env for the mongodb name?
this.global.__MONGO_DB_NAME__ = __MONGO_DB_NAME__;
await super.setup();
}
async teardown() {
console.log('Teardown MongoDB Test Environment');
await Promise.all([super.teardown(), this.__MONGOD__.close()]);
} EDIT
That could become pretty huge for projects with a big amount of db related tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless if it's feasible to spin up a db per test, this change is better. We can always change again later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dope, thanks for taking a look @vladgolubev
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
The example was misleading because it would only work when running a single test or with the option
--runInBand
. This is a fix to get around the fact that all tests run in their own contexts.Summary
Test plan