test: write concern test cleanup

[emadum]

Description

Cleans up duplication and deprecation warnings in write concern command construction tests.

What changed?

Are there any files to ignore?

[mbroadst]

I agree with you that these tests are a whole bunch of copy-pasta! Your approach looks similar to the ReplSetFixture we used in some of our core test, but I wonder if we can design a better way out of this situation..

Ultimately, we are testing command construction here. The tests boil down to something like the following:

client.connect((err, client) => {
  expect(err).to.not.exist;
  const db = client.db(this.configuration.db);

  db.collection('test')
    .aggregate([{ $match: {} }, { $out: 'someOtherDb' }], {
      w: 2,
      wtimeout: 1000
    })
    .toArray(err => {
      expect(err).to.not.exist;
      expect(constructedCommand)
        .property('writeConcern').to.eql({ w: 2, wtimeout: 1000 });

      client.close(done);
    });
});

where constructedCommand is captured during execution. We don't need a mock server simulating a replicaset here, or even a mock server at all! Sometimes I think the mock server was written before a time when tools like sinon made this easy.

It might be a bit tricky to figure out at first, but I think we can use some existing examples of stubbing the Topology/Server classes in order to provide the same functionality without actually setting up TCP sockets. Alternatively, we could convert these to actual functional tests, connect them to real servers, and use command monitoring to verify that the writeConcern is constructed properly. I think both are fine solutions, and better than what we've got in the source today

[emadum]

Your approach looks similar to the ReplSetFixture we used in some of our core test, but I wonder if we can design a better way out of this situation..

Haha yeah as I was writing it I was thinking "this is a very general pattern, someone has probably done this before somewhere." Creating the class was simple though, so I didn't spend time grepping. The laborious part was stripping the boilerplate - it should be trivial to convert these tests to a better underlying paradigm now.

It might be a bit tricky to figure out at first, but I think we can use some existing examples of stubbing the Topology/Server classes in order to provide the same functionality without actually setting up TCP sockets.

Agreed, this sounds like a better solution.

Alternatively, we could convert these to actual functional tests, connect them to real servers, and use command monitoring to verify that the writeConcern is constructed properly.

I'm not sure I see any benefit to that, since these tests feel like they should be unit tests since the server they are running against is irrelevant. I could see a future optimization to our evergreen setup where we only run unit tests once per environment, and then we'd have an incentive to keep tests that don't really need a server from using one.

[mbroadst]

Great, glad we agree on the stubbed approach. I think we could very likely end up with a fantastic new helper like:

it('should build writeConcern properly', withStubbedClient((client, commands, done) => {
  client.connect((err, client) => {
    expect(err).to.not.exist;
    const db = client.db(this.configuration.db);
  
    db.collection('test')
      .aggregate([{ $match: {} }, { $out: 'someOtherDb' }], {
        w: 2,
        wtimeout: 1000
      })
      .toArray(err => {
        expect(err).to.not.exist;
        expect(commands)
          .property('[0].writeConcern').to.eql({ w: 2, wtimeout: 1000 });
  
        client.close(done);
      });
  });
});

I'm sure there's something I'm missing here though, here are some thoughts that occur to me immediately:

• commands would have to track all built wire protocol commands, but I think at this point it might be enough to stub command on the connection or something).
• you might not want every one of those commands 🤷

[emadum]

we could very likely end up with a fantastic new helper

Yeah that would be awesome! I've refactored all of the tests to use a writeConcernTest helper, which in turn uses a withStubbedClient helper, which just wraps around the existing WriteConcernTest class. However, it should now be really easy to simply update the helpers without further modifying the tests.

[mbroadst]

Are you going to further refactor this to use sinon or are we deferring that work to a later PR?

[emadum]

Yeah I was planning to follow up by extracting withStubbedClient to test/functional/shared.js and making it use sinon.

[mbroadst]

follow up in this PR or a subsequent one? Just trying to decide on whether to approve or not :)

[emadum]

There's no rush to get this cleanup in so might as well do it in this PR, I'll mark it as a draft for now.

[emadum]

Created a ticket to track this work: NODE-2603

[emadum]

Given we're probably not getting to NODE-2603 particularly soon, I think it's a good idea to merge this PR in for now for the boilerplate reduction, so it doesn't sit around any longer. cc @nbbeeken