Skip to content

feat(NODE-4559): add mongodb-legacy metadata #5

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Sep 12, 2022
Merged

feat(NODE-4559): add mongodb-legacy metadata #5

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ff54a8f
feat(NODE-4559): add mongodb-legacy metadata
nbbeeken Aug 30, 2022
c05f31c
test: check for metadata set
nbbeeken Sep 6, 2022
94b2b38
add version, prepend
nbbeeken Sep 6, 2022
a4dd864
organize tests into blocks
nbbeeken Sep 7, 2022
62182b3
test: organization
nbbeeken Sep 8, 2022
b1d8b9a
test: name fixes
nbbeeken Sep 8, 2022
33845a6
Apply suggestions from code review
nbbeeken Sep 9, 2022
816c1bb
improve nullish handling and addLegacyMetadata logic
nbbeeken Sep 9, 2022
ebe769c
fix coverage
nbbeeken Sep 9, 2022
39e6c55
fix: do not mutate input options
nbbeeken Sep 9, 2022
0274944
clarify title
nbbeeken Sep 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/legacy_wrappers/mongo_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,18 @@ Object.defineProperty(module.exports, '__esModule', { value: true });

module.exports.makeLegacyMongoClient = function (baseClass) {
class LegacyMongoClient extends baseClass {
// constructor adds client metadata before constructing final client
constructor(connectionString, options) {
options = { ...options };
if (options.driverInfo != null && typeof options.driverInfo.name === 'string') {
options.driverInfo.name += '|mongodb-legacy';
} else {
options.driverInfo = { name: 'mongodb-legacy' };
}

super(connectionString, options);
}

static connect(url, options, callback) {
callback =
typeof callback === 'function'
Expand Down
18 changes: 18 additions & 0 deletions test/unit/legacy_wrappers/mongo_client.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,24 @@ describe('legacy_wrappers/mongo_client.js', () => {
await client.close();
});

describe('client metadata', () => {
it('should set mongodb-legacy to the client metadata', () => {
Copy link
Collaborator

@baileympearson baileympearson Sep 7, 2022

Choose a reason for hiding this comment

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

Can we structure these tests a bit better? I'm thinking context blocks explaining the context for the test, followed by a better description ("set mongodb-legacy to the client metadata" isn't what happens, and isn't descriptive). For example

context('when no driverInfo is passed to the MongoClient constructor', () => {
  it('sets the driver name to mongodb-legacy', () => {});
  it(`sets the version to ${version}`, () => {}
});

I think this (or a similar structure) would apply to every new test added in this PR

Copy link
Contributor Author

@nbbeeken nbbeeken Sep 7, 2022

Choose a reason for hiding this comment

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

Organized!

Copy link
Collaborator

@baileympearson baileympearson Sep 8, 2022

Choose a reason for hiding this comment

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

three comments

  • the descriptions for the tests aren’t great. for example - what does should set mongodb-legacy to the client metadata mean? the test description should describe what the test is testing, should assign client metadata.name to mongodb-legacy . this leads into the second comment
  • the tests would be better organized with a single assertion per it block. should assign client metadata.name to mongodb-legacy and client metadata.version to the version in package.json is a mouthful, but if it’s split into two separate it blocks, should assign client metadata.name to mongodb-legacy and should assign client metadata.version to the version in the package.json are both easy to understand and describe exactly what the test is testing
  • the other tests would be much more understandable if we broke it block into a context + it (preferably two its actually). for example
it('should prepend mongodb-legacy to user passed driverInfo.name', () => {
        const client = new LegacyMongoClient(iLoveJs, { driverInfo: { name: 'mongoose' } });
        expect(client.options.metadata).to.have.nested.property(
          'driver.name',
          'nodejs|mongodb-legacy|mongoose'
        );
        expect(client.options.metadata)
          .to.have.property('version')
          .that.includes(currentLegacyVersion);
      });

would become

context('when driverInto.name is provided and driverInfo.version is not', () => { 
  it('should prepend mongodb-legacy to user passed driverInfo.name', () => {...}
  it('should assign driverInfo.version to the version in the package.json', () => { ... }
});

Copy link
Contributor Author

@nbbeeken nbbeeken Sep 8, 2022

Choose a reason for hiding this comment

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

organized more!

const client = new LegacyMongoClient(iLoveJs);
expect(client.options)
.to.have.nested.property('metadata.driver.name')
.to.be.a('string')
.that.includes('mongodb-legacy');
});

it('should add mongodb-legacy to existing driverInfo.name', () => {
const client = new LegacyMongoClient(iLoveJs, { driverInfo: { name: 'mongoose' } });
expect(client.options)
.to.have.nested.property('metadata.driver.name')
.to.be.a('string')
.that.includes('mongoose|mongodb-legacy');
});
});

it('calling MongoClient.connect() returns promise', async () => {
sinon.stub(mongodbDriver.MongoClient.prototype, 'connect').returns(Promise.resolve(2));
expect(client).to.have.property('connect').that.is.a('function');
Expand Down