Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SyncProducer.SendMessages doesn't work properly in tests #776

Closed
luismfonseca opened this issue Nov 1, 2016 · 2 comments
Closed

SyncProducer.SendMessages doesn't work properly in tests #776

luismfonseca opened this issue Nov 1, 2016 · 2 comments

Comments

@luismfonseca
Copy link

Versions

Please specify real version numbers or git SHAs, not just "Latest" since that changes fairly regularly.
Sarama Version: 02452b3987ac0e7b93702149e96d4f9b2e6521ec
Kafka Version: not applicable
Go Version: go version go1.7.3

Problem Description

The MR #677 brought the SendMessages function and its mock counterpart. Now the test didn't really test assertions in the form of:

		producer.ExpectSendMessageAndFail(sarama.ErrOutOfBrokers)

So somewhere in the history of this file, it stopped working.
If I change the mock to use the sister function SendMessage, my tests work:

func (sp *SyncProducer) SendMessages(msgs []*sarama.ProducerMessage) error {
	for _, msg := range msgs {
		_, _, err := sp.SendMessage(msg)
		if err != nil {
			return err
		}
	}
	return nil
}

But I don't know if that would be the correct fix. Another way to fix this just by editing this line:

		expectations := sp.expectations[0 : len(msgs)-1]

to

		expectations := sp.expectations[0 : len(msgs)]

If there's 1 message, a slice from 0 to 1 - 1 is going to be an empty slice.
But this fix does not make the SendMessages actually use the expectation.CheckFunction.

I can write a minimal test for this if this description was too confusing, and a merge request if someone tells me the preferred way to fix this.

@luismfonseca
Copy link
Author

luismfonseca commented Nov 15, 2016

Can I make a merge request changing:
expectations := sp.expectations[0 : len(msgs)-1]
to
expectations := sp.expectations[0 : len(msgs)]
?

@eapache
Copy link
Contributor

eapache commented Nov 22, 2016

Nice catch on both issues, I'd be happy to take pull requests to fix them.

I think your suggestion to just loop and call SendMessage is the best fix since it will then automatically pick up any future changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants