Expose interfaces to public API to ease testing

[emaincourt]

Hi,

I’ve been using NSQ in production for more than 3 years now and I sincerely want to thank you guys for the great work you’ve been doing.

This PR aims to expose newly created interfaces for Conn, Consumer and Producer, with the goal of making testing simpler. Please note that this PR does not break the current API in any way and that all the changes have been made with respect to the documentation.

I know that this point was already discussed in #146 and I saw that PRs were welcome. So if it’s still the case and you think this kind of modification could be in respect with the roadmap, I would be more than glad to fix everything that needs to be in this PR. Otherwise, I can just close if. Just let me know if all this does not make any sense to you.


Please find below the definition of the main interfaces. More private ones are in the code. Obviously all the namings can be discussed. This was not the purpose of my contribution.

type Conn interface {
    fmt.Stringer

    LoggerCarrierSetters

    Connect() (*IdentifyResponse, error)
    RemoteAddr() net.Addr
    Read(p []byte) (int, error)
    Write(p []byte) (int, error)
    WriteCommand(cmd *Command) error
    RDY() int64
    SetRDY(rdy int64)
    MaxRDY() int64
    LastRDY() int64
    LastRdyTime() time.Time
    LastMessageTime() time.Time
    Flush() error
    Close() error
    IsClosing() bool
}

type Producer interface {
    fmt.Stringer

    // Please note that those 3 prototypes could potentially be removed from this interface

    // and migrated somewhere else, see below
    SetLogger(l logger, lvl LogLevel)
    SetLoggerLevel(lvl LogLevel)
    SetLoggerForLevel(l logger, lvl LogLevel)

    PublishAsync(
        topic string,
        body []byte,
        doneChan chan *ProducerTransaction,
        args ...interface{},
    ) error
    MultiPublishAsync(
        topic string,
        body [][]byte,
        doneChan chan *ProducerTransaction,
        args ...interface{},
    ) error
    Publish(topic string, body []byte) error
    MultiPublish(topic string, body [][]byte) error
    DeferredPublish(topic string, delay time.Duration, body []byte) error
    DeferredPublishAsync(
        topic string,
        delay time.Duration,
        body []byte,
        doneChan chan *ProducerTransaction,
        args ...interface{},
    ) error

    Ping() error
    Stop()
}

type Consumer interface {

    // Please note that those 3 prototypes could potentially be removed from this interface

    // and migrated somewhere else, see below
    SetLogger(l logger, lvl LogLevel)
    SetLoggerLevel(lvl LogLevel)
    SetLoggerForLevel(l logger, lvl LogLevel)

    ConnectToNSQLookupd(addr string) error
    ConnectToNSQLookupds(addresses []string) error
    ConnectToNSQDs(addresses []string) error
    ConnectToNSQD(addr string) error
    DisconnectFromNSQD(addr string) error
    DisconnectFromNSQLookupd(addr string) error

    AddHandler(handler Handler)
    AddConcurrentHandlers(handler Handler, concurrency int)

    Stats() *ConsumerStats
    SetBehaviorDelegate(cb interface{})

    IsStarved() bool
    ChangeMaxInFlight(maxInFlight int)

    Stop()
    GetStopChan() chan int
}

I also did my best to factorize code as much as possible in the logging part. Some code was duplicated across both the consumer.go, producer.go and conn.go, so I thought it might be interesting to gather everything into its own interface, and a default implementation.

If this is something you might be interested in, the limiting part of this side is that the public API of Conn exposes a different prototype for both SetLogger and SetLoggerForLevel, compared to the ones exposed by Consumer and Producer.

Please note that this could be refactorized pretty easily, breaking the API but without any functional changes (e.g. using a default value for format).

Thank you for taking the time to read and consider this PR

[jehiah]

@emaincourt Glad to hear NSQ has been working well for you!

Is there any context you can share about the types of testing you are trying to accomplish? I'm surprised you have testing that covers the connection level.

In the interest of sharing, i've updates https://gist.github.com/jehiah/45af9135b6ecf5537646 with the test package I use for testing; I've found this to be sufficient for the producer side, and the handler side (I never really have had a need to test/mock the Consumer, just testing around the handler.

[emaincourt]

Hi @jehiah !

Thanks for taking the time to answer and sharing your gist. Regarding to this file I think we're both talking about the same kind of testing.

To give you a bit more context, not exposing interfaces makes it difficult to achieve a great isolation level when running unit tests. Nowadays and up to my best understanding, there are two possibilities for people wanting to unit test a system relying on NSQ. Please correct me if I'm wrong:

• Wrap the NSQ client into a custom interface, more or less what you do in your gist, which seems to be the better working scenario
• Dynamically start a NSQ server

Well in the first case, exposing interfaces from the client would prevent from copy/pasting pieces of code again an again, or having some utils somewhere. The library could expose mocks or only let people generate their own.

Which brings us to the second point: start a server during tests. Maybe I'm too confident but I tend to trust NSQ and its go library enough not to test its functionalities. This is exactly why I do not have tests on the connection level 🙂

When running unit tests I believe isolation level between components is pretty important. Starting a server during tests (or two for nsqd and nsqlookup) might slow down tests, increase friction, and is not of really great value. As you said, it would be very surprising to test this part.

As you also said, one is rarely testing the consumer when writing tests for the producer, so you don't really want to test if the message is gonna be available on the other side. You better check that you properly transformed you data if needed, handled all cases and called the right methods with the right params on the library (e.g. dlq on errors).

Does all that make any sense to you ? What do you think ?

Thank you for your time