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

Improve expectation API to allow for testing of more intricate scenarios #298

Closed
wants to merge 7 commits into from

Conversation

wvanbergen
Copy link
Contributor

This makes the API for mock broker expectations a bit more powerful.

  • Allow setting a latency for the response (this replaces the previous broker-wide latency setting)
  • Allow setting a Before and After callback around the response.

Use these capabilities to add some tests:

  • The first client tests asserts that if one broker times out, it will continue to use the second broker to discover metadata.
  • The second client test will make sure that eventually NewClient will return an error if all brokers have high latency.
  • Test the producer with multiple batches in flight at different retry levels.

@Shopify/kafka

}

func (b *MockBroker) SetLatency(latency time.Duration) {
b.latency = latency
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can now be set separately for every expectation/response.

@wvanbergen wvanbergen changed the title Add some tests around Client metadata timeouts. Improve MockBroker expectation API to allow for more intricate scenarios Feb 24, 2015
@wvanbergen wvanbergen changed the title Improve MockBroker expectation API to allow for more intricate scenarios Improve expectation API to allow for testing of more intricate scenarios Feb 24, 2015
func (b *MockBroker) Returns(response encoder) *MockBrokerExpectation {
expectation := &MockBrokerExpectation{response: response}
b.expectations <- expectation
return expectation
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now returns itself so the methods below can be chained.

Technically this is a bit shitty, because we immediately add it to the expectations channel, but allow it to be altered later. I guess this could cause concurrency issues, but because of the synchronous way our tests are set up I don't think this will cause issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blegh, the race detector is on to me. I guess this needs fixing after all. API suggestions?

@wvanbergen
Copy link
Contributor Author

CI is failing due to kisielk/errcheck#62

@wvanbergen wvanbergen force-pushed the client_metadata_latency_tests branch from 25d4d2f to 37d9cb7 Compare February 24, 2015 11:02
@wvanbergen
Copy link
Contributor Author

This is failing on Travis, even though this works fine locally:

--- FAIL: TestSlowCluster (0.12 seconds)
    allocs.go:1: write tcp 127.0.0.1:46446: broken pipe
    allocs.go:1: write tcp 127.0.0.1:47507: broken pipe
    allocs.go:1: write tcp 127.0.0.1:55751: broken pipe

any ideas?

@wvanbergen
Copy link
Contributor Author

I assume this is caused by the mock broker trying to write a response during/after the reader timeout of the client.

@wvanbergen wvanbergen force-pushed the client_metadata_latency_tests branch from 5254546 to 7f9e00a Compare February 24, 2015 13:33
@wvanbergen
Copy link
Contributor Author

Miraculously, it works by adding a parameter on whether to expect errors to the expectation.

@wvanbergen
Copy link
Contributor Author

Hmmm, in some cases is the read timeout appears to be triggered before the mock broker has figured out a request is incoming, and so it won't resolve the expectation. Any ideas of making this more robust, compared to simply raising the read timeout value?

}

func (b *MockBroker) SetLatency(latency time.Duration) {
b.latency = latency
type MockCluster map[int32]*MockBroker
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just a slice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure the index matches the BrokerID. Broker IDs are 1-indexed, while slice indices start at 0.

@eapache eapache mentioned this pull request Feb 24, 2015
4 tasks
Make expectation a separate struct instead of just a response. This type can then be used to have callbacks and other properties, so we can simulate concurrency more easily.
@wvanbergen wvanbergen force-pushed the client_metadata_latency_tests branch from 7f9e00a to 1a44810 Compare February 25, 2015 12:04
@eapache
Copy link
Contributor

eapache commented Mar 11, 2015

This will not be merged in its current form - #312 should be fixed first.

@eapache eapache closed this Mar 11, 2015
@wvanbergen wvanbergen deleted the client_metadata_latency_tests branch May 6, 2015 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants