mocktools / go-smtp-mock

SMTP mock server written on Golang. Mimic any 📤 SMTP server behavior for your test environment with fake SMTP server.
MIT License
129 stars 18 forks source link

[FEATURE] Ability to wait for server messages #181

Open nicolasparada opened 8 months ago

nicolasparada commented 8 months ago

New bug checklist

Bug description

Upgraded from v2.2.1 to v2.3.0 and our tests started failing. Our setup is quite simple. We use it as a mock during testing. We start the mock server on TestMain() and use it on the tests. The test consist on sending a mail using the standard net/smtp using the mock server address. Then reading the messages, finding the message we want and doing some assertions.

We couldn't find the expected messages using .Messages().

I suspects there is a race condition there. Since the tests do pass locally, but they fail during CI (probably due to a slower machine on CI). I added a quick time.Sleep(time.Millisecond) before accessing .Messages() and that caused the tests to pass again.

bestwebua commented 8 months ago

Hi, @nicolasparada! Please add usage examples of using smtpmock here. @mitar Do you have ideas how we can assist with it?

mitar commented 8 months ago

I think this is expected. This is another (also present before) race condition between sending and reading. Currently we do not have memory issues, but logical race condition is still present. We are just clearer about it because we do not expose any data until sending is finished. In my own tests, I also have sleep before reading .Messages.

I think we should add an assertion to this package itself, something similar to what #154, which blocks until expected number of messages arrive, or timeout.

So something like:

.ExpectMessages(number, timeout)

which return message and error.

bestwebua commented 3 days ago

@mitar We need something like in an example below, am I right?

// WaitForMessages waits for the specified number of messages to arrive or until timeout is reached.
// Returns the messages and an error if timeout occurs before receiving expected number of messages.
func (server *Server) WaitForMessages(count int, timeout time.Duration) ([]Message, error) {
    deadline := time.Now().Add(timeout)
    for {
        messages := server.Messages()
        messagesCount = len(messages)
        if messagesCount >= count {
            return messages, nil
        }

        if time.Now().After(deadline) {
            return messages, fmt.Errorf("timeout waiting for %d messages, got %d", count, messagesCount)
        }

        time.Sleep(1 * time.Millisecond)
    }
}

messages, err := server.WaitForMessages(1, 2*time.Millisecond)

I guess we also need something like this one but for MessagesAndPurge() => WaitForMessagesAndPurge(). Thoughts?

mitar commented 2 days ago

This is probably fine, but it is busy-waiting. For testing it is probably OK. But it could also be implemented with Cond, but then we would need to have some tracking of who is waiting and update that when messages are added.

bestwebua commented 2 days ago

This is probably fine, but it is busy-waiting. For testing it is probably OK. But it could also be implemented with Cond, but then we would need to have some tracking of who is waiting and update that when messages are added.

Thanks for explanation! I got your point of view and agree. But for testing purpose should we complicate current implementation? Maybe we can simplify current using like in your propose?

mitar commented 2 days ago

I think it would be fine for now and we can improve the implementation in the future.