Use interfaces for Channel/Connection for mock testing capability

[bbangert]

At the moment, the library utilizes structs everywhere. This means that using amqp makes mocking for tests quite difficult since a real AMQP broker is required. In our code I created an interface that the Channel/Connection meets, so that I could use my own mocks, but this was a bit more work than necessary (if the amqp lib itself used interfaces).

Ideally the Channel, and Connection should be interfaces so that they can be mocked, from my look at the code only a few other methods not currently exposed as the public API need an interface definition (the Config and raw frame Send). This way mocking the channel/connection can be done so that a real AMQP server isn't necessary, it'd also make it easier for the library itself to have a more thorough set of tests.

[streadway]

We ran into a similar use case in some code that was trying to mock Delivery and Publishing structs. I took the bias in this library to be tightly coupled to the protocol, flattening it to a single implementation. As a result the number of methods reflects the protocol which is quite wide.

The direct use of the Connection/Channel structs is usually (at least from what i've done/seen) abstracted into higher level Setup/Publish or Setup/Consume interfaces in the application code. These much smaller interfaces that live in your application provide the mapping between your application and the wire, keeping the library types contained in your adapter. The implementation of your Publish/Consume interface would interact with your Setup to handle reconnect/recovery cases. Your application could choose to expose a io.Reader io.Writer interface that is implemented in terms of pulling off of a amqp.Channel.Consume() chan or publishing via amqp.Channel.Publish() that would then be easier to mock.

Regarding the tests, there is a balance between working directly with structs and having an interface for every API boundary. I chose to acceptance test based on the expected protocol rather than on internal interface boundaries. These tests mock the server at the protocol level, which is of course too low level for an application to reuse.

Do you have some examples that communicate what you'd like to see from an application developer's point of view?

Unit testing is still a bit weak, leaning heavily on integration tests for coverage. Do you have a list of tests you'd like to see that could only be implemented by using internal interfaces?

[bbangert]

Sure, in my amqp plugin to our heka project I made most of my methods rely on getting objects that implement either the Connection or Channel interface I defined:
https://github.com/mozilla-services/heka/blob/features/amqp-plugin/pipeline/amqp_plugin.go#L26

We also have a connection hub responsible for doling out Channels for a URI, because in our case we have multiple connections setup for input/output in separate goroutines and wanted to multiplex the channel still (without having to pass the connection all over). By having the connection hub have an interface, as well as returning the AMQPChannel interface we defined, we're able to mock most everything since all our code utilizes is methods on Channel for the most part. The one bit we couldn't mock was the Message.Ack(), because its a struct.

Here's the tests thus far showing the use of our mocks:
https://github.com/mozilla-services/heka/blob/features/amqp-plugin/pipeline/amqp_plugin_test.go#L50

The bit that got tricky is that since Mesage.Ack() calls up to the connection, and we didn't replace it with a mock, it hits a nil pointer since there is no real connection. If amqp internally used the AMQPChannel or AMQPConnection interface, then I could just capture its attempted usage of the connection in Ack, otherwise I'm stuck doing what I do at the moment (test for the message having a Testing flag and not calling Ack in that case, sigh).

Using interfaces for these might also make the code a bit nicer since at the moment the various structs poke around in each other quite deeply (the message method poking into the channels internal connection reference).

As you mention, I could write an adapter that wraps this up, but that more or less is what this entire plugin is, and there's the question of how you actually test the adapter... which is going to execute all the amqp calls.

[streadway]

I designed the initial API under the assumption that the handler of a Delivery would not have a reference to the Channel. Something that could be run concurrently with the rest of your application:

func consume(in <-chan amqp.Delivery, out chan MyPayload) {
  for d := range in {
    result, err := convertToMyPayload(d)
    if err != nil {
      close(out)
      retrun // aka crash
    }
    output <- result
    d.Ack()
  }
}

payloads := make(chan MyPayload)
msgs, _ := amqp.Consume(/* lolmqp fields */)
go consume(msgs, payloads)
go process(payloads)

This makes process(payloads) unit testable, where "consume" would then be mocked in the application.

But your example needs more state to do its work and still holds on to the reference to the amqp.Channel (or AMQPChannel) in the same scope as Delivery.Ack().

I see some approaches:

• Keep the Delivery type a struct and move the Ack/Nack/Reject/Cancel methods to the Channel exposing the nuances of DeliveryTag/Multiple
• Turn Delivery into an interface type and move the 21 wire format fields into accessors
• Export the channel field as amqp.Delivery.Channel (or similar) which would implement Ack/Nack/Cancel/Reject with the implementation provided by amqp.Channel.Consume

I'll need to sleep on this and appreciate your feedback.

[bbangert]

Having a consume function like that add's its own overhead of course. You now have another goroutine in between the actual AMQP deliver channel that pulls them all off, then redelivers them to a new channel translating the types.

I think your first approach listed would be a nice one as I don't need to worry about mocking the Delivery struct. All I'd need to do is mock the Channel.Ack/Nack method, and its easier to just have an interface for the Channel/Connection, rather than creating a bunch of getters for Deliver so that it can be turned into an interface. Looking through the code, the Channel only really uses two currently private methods of Connection, if they were exposed under an interface, it'd be pretty easy to mock the whole setup if Connection/Channel were interfaced, and your first approach was done.

[streadway]

Having a consume function like that add's its own overhead of course. You now have another goroutine in between the actual AMQP deliver channel that pulls them all off, then redelivers them to a new channel translating the types.

We use this pattern our application code, and it doesn't produce a bottleneck. The small scopes of concurrent processing step increases the readability and reasonability which is what we often optimize for.

I think your first approach listed would be a nice.

I did the first and third. I kept the Ack/Nack interface on the delivery because we use a publish one, subscribe many pattern internally to scale out our messaging tier for partially ordered deliveries. It's nice to fan the deliveries from the multiple channels back in to a single chan Delivery from which the handler can range over.

I chose to interface only the Acknowledgement of the delivery, rather than the entire channel as acknowledgement is the most common use case, and the rest can be satisfied with a reference to the application's messaging context along with the delivery meaning the consumers would range over an application type rather combining the two.

Please move the discussion over to #65 if this works for you.