Add private Net::LDAP::Connection#write_request

[jch]

This new method accomplishes two things:

• DRY up existing calls to #write
• provide an entry point for test mocking what goes out on the wire

While working on #115, I noticed it was difficult to make assertions about what was being written out on the socket because those values were already BER encoded. By adding #write_request, it allows us to write assertions like the following:

# edited for typo
def test_some_operation
  mock = Minitest::Mock.new
  mock.expect(:write, true, [[1.to_ber, "some_operation's data"].to_ber_sequence])
  conn = Net::LDAP::Connection.new(:socket => mock)
  conn.some_operation(...)
end

It's possible to go even more general, but this refactoring is small and does what I want. I also introduced Minitest::Mock here. I saw that the project also has flexmock as a dependency, but proposed it with minitest in case that's the direction we want to move towards. If it's preferred, I can also figure out how to achieve the same mocking with flexmock.

cc @mtodd @schaary

[mtodd]

@jch do you think it might make sense to bake the write_request logic into the write method instead of adding another layer of logic? Don't want to overburden Net::LDAP::Connection#write but it's super slim right now, and if every call to write is wrapped in write_request, then it might make sense to inline this method.

[mtodd]

👌

[mtodd]

No problems with using minitest/mock in general. Not a huge fan of adding more inconsistency, but if we're all 👍 to moving towards minitest and getting rid of flexmock (which I think we're all in agreement on), works for me.

[jch]

do you think it might make sense to bake the write_request logic into the write method instead of adding another layer of logic?

Works for me. I started with #write_request because I wasn't sure if there were other callers for this interface, but that's unlikely seeing how it's private. It also doesn't affect existing behavior, so I don't see a problem there either.

[jch]

@schaary @mtodd ready for more 👀

[mtodd]

@jch 👍 diff looks good. Wishing we had integration tests right now. Would like to verify this against a directory service (AD or OpenLDAP) to be safe. Can do this later this week if you don't get a chance.

[jch]

👍 I'll hold off merging until we have some basic integration tests then.

[mtodd]

Assigned myself since you're blocked on integration testing to move forward on this.

[mtodd]

@jch added basic integration tests in #129, want me to merge them in and add integration tests for this? Or want to tackle integration tests?

[jch]

@mtodd what integrations tests do you have in mind?

Seeing how #write is internal and private, I think the unit tests may suffice.

[mtodd]

@jch looks like this mock was missed.

[mtodd]

@jch technically, all operations go through write so the existing integration tests technically cover this change sufficiently; I was thinking it would be a good opportunity to add another integration test to expand overall coverage. For instance, we're missing basic integration tests on almost all of the operations touched here (all except for bind and search). I wouldn't block this PR on 'em, FWIW.

[jch]

@mtodd gotcha. 👍 for adding more integration tests for all the public methods on Net::LDAP, but agree that we can do that in a separate PR. I'll fix up the CI issue and get this merged.