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

Close all connections in all tests, also change some tests to be working stand-alone #12890

Merged
merged 11 commits into from
Jan 17, 2023

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/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.

@@ -8,8 +8,15 @@ const assert = require('assert');
const mongoose = start.mongoose;

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

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

const db = mongoose.createConnection(start.uri, {});

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.

@@ -25,6 +25,8 @@ describe('Model', function() {
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