Skip to content

Conversation

@hasezoey
Copy link
Collaborator

@hasezoey hasezoey commented Jan 8, 2023

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:

  • all test files except connection.test.js and index.test.js have been updated with connectionsToClose or a simple mongoose.disconnect instead of not closing the connections
  • some spaces have been added between some tests
  • many test/helpers test files have been updated to work when tested stand-alone (only testing a single test file)
  • change some import order in test/helpers to be consistent across test/helpers test files
  • some tests have been updated to use the existing connection instead of creating a new one (when it was not about using a new connection)

these 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/[email protected]/node/internal/errors.ts:113:11)
    at __node_internal_exceptionWithHostPort (https://deno.land/[email protected]/node/internal/errors.ts:295:12)
    at TCPConnectWrap._afterConnect [as oncomplete] (https://deno.land/[email protected]/node/net.ts:369:16)
    at TCP.afterConnect (https://deno.land/[email protected]/node/internal_binding/connection_wrap.ts:71:11)
    at https://deno.land/[email protected]/node/internal_binding/tcp_wrap.ts:375:16
error Command failed with exit code 1.

describe('collections:', function() {
const connectionsToClose = [];

after(async function() {
Copy link
Collaborator

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.

Copy link
Collaborator Author

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


assert.strictEqual(db2, db3);
db.close();
db.close(done);
Copy link
Collaborator

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.

assert.equal(db.shouldAuthenticate(), false);

db.close();
db.close(done);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same, avoid adding done.

let Comments;
let BlogPost;

const connectionsToClose = [];
Copy link
Collaborator

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().

Copy link
Collaborator

@vkarpov15 vkarpov15 left a 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 👍

@vkarpov15 vkarpov15 added this to the 6.8.4 milestone Jan 17, 2023
@vkarpov15 vkarpov15 merged commit 256a11e into Automattic:master Jan 17, 2023
@hasezoey
Copy link
Collaborator Author

I'm just going to merge this one and make suggested changes in separate PR. Thanks +1

also good, just wanted to know before how to handle multiple connections in a test with the suggested way, because a single let db = connection would not be sufficient and multiple let db1 = connection; let db2 = connection; would with even more connections become redundant

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.

2 participants