-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Close all connections in all tests, also change some tests to be working stand-alone #12890
Conversation
to prevent connections from leaking across tests
@@ -8,8 +8,15 @@ const assert = require('assert'); | |||
const mongoose = start.mongoose; | |||
|
|||
describe('collections:', function() { | |||
const connectionsToClose = []; | |||
|
|||
after(async function() { |
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.
Instead of close()
in an after
, why not just declare let db = null
at top-level describe block, and then afterEach(() => db && db.close())
? That way tests don't need an extra connectionsToClose.push()
call, just replace const db = mongoose.createConnection()
with db = mongoose.createConnection()
? That would avoid the need for extra boilerplate in 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.
i did this to not modify the test too much, also some tests use multiple connections
const db = start(); | ||
const db2 = db.useDb(start.databases[1], { useCache: true }); | ||
const db3 = db.useDb(start.databases[1], { useCache: true }); | ||
|
||
assert.strictEqual(db2, db3); | ||
db.close(); | ||
db.close(done); |
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.
Let's not add extra done()
usage, we should avoid using done()
. return db.close()
should be fine because db.close()
returns promise if no callback specified.
const db = mongoose.createConnection(start.uri, {}); | ||
|
||
assert.equal(db.shouldAuthenticate(), false); | ||
|
||
db.close(); | ||
db.close(done); |
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.
Same, avoid adding done.
@@ -25,6 +25,8 @@ describe('Model', function() { | |||
let Comments; | |||
let BlogPost; | |||
|
|||
const connectionsToClose = []; |
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.
Let's do let db = null;
instead and close()
in an afterEach()
.
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'm just going to merge this one and make suggested changes in separate PR. Thanks 👍
also good, just wanted to know before how to handle multiple connections in a test with the suggested way, because a single |
test: some cleanup re: #12890
Summary
This PR updates some tests / test files to close all connections that were opened, also changes some test files to be working stand-alone
though i am not really proud of some of the connection caching done with
connectionsToClose
some more detailed notes:
connection.test.js
andindex.test.js
have been updated withconnectionsToClose
or a simplemongoose.disconnect
instead of not closing the connectionstest/helpers
test files have been updated to work when tested stand-alone (only testing a single test file)test/helpers
to be consistent acrosstest/helpers
test filesthese changes are required for a follow-up PR i am doing to integrate MMS to the deno tests too (instead of manually downloading and starting mongodb), because otherwise it would fail with the following when the single-instance is stopped:
error: Uncaught Error: connect ECONNREFUSED 127.0.0.1:36683 - Local (undefined:undefined) Error.captureStackTrace(err); ^ at __node_internal_captureLargerStackTrace (https://deno.land/std@0.171.0/node/internal/errors.ts:113:11) at __node_internal_exceptionWithHostPort (https://deno.land/std@0.171.0/node/internal/errors.ts:295:12) at TCPConnectWrap._afterConnect [as oncomplete] (https://deno.land/std@0.171.0/node/net.ts:369:16) at TCP.afterConnect (https://deno.land/std@0.171.0/node/internal_binding/connection_wrap.ts:71:11) at https://deno.land/std@0.171.0/node/internal_binding/tcp_wrap.ts:375:16 error Command failed with exit code 1.