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

Further mockbrocker improvements #492

Merged
merged 2 commits into from
Aug 6, 2015
Merged

Conversation

horkhe
Copy link
Contributor

@horkhe horkhe commented Jul 23, 2015

This PR brings further improvements into the mockbrocker:

  • A set of response builders were introduced to construct responses based on actual request data. Note that builders can respond to the most common request parameters. If something very specific is needed then a response can always be constructed directly;
  • mockbroker.SetHandlerByMap method was introduced that provides a declarative way of defining a request handler that use mockresponse builders.
  • consumer_test.go was rewritten using the new test primitives. Note that it is not a direct translation. Some tests were split for better isolation of concerns. Note also that TestConsumerRebalancingMultiplePartitions is using time delays for synchronization and therefore race prone. It could have been rewritten in almost non-racy way by removing the goroutine that reads from partition consumer channels and putting those reads among the lines of the main test flow. But I opted to leave it this way, because the goroutine code resembles how PartitionCosnumer is used in real life.

@horkhe horkhe force-pushed the maxim/stash branch 5 times, most recently from e3636c9 to f130b98 Compare July 27, 2015 18:29
@horkhe horkhe force-pushed the maxim/stash branch 4 times, most recently from 5b98f0c to 2a9b0bc Compare August 4, 2015 18:56
@horkhe
Copy link
Contributor Author

horkhe commented Aug 4, 2015

@eapache it builds now that I rebased it on the latest master.

- A set of response builders were introduced to construct
  responses based on a particular request parameters.
- mockbroker.SetHandlerByMap that provides a neat way of
  defining a request handler that use mockresponse builders.
- consumer_test.go was rewritten using the new test primitives.
@@ -1,7 +1,7 @@
default: fmt vet errcheck test

test:
go test -v -timeout 60s -race ./...
go test -v -timeout 2m -race ./...
Copy link
Contributor

Choose a reason for hiding this comment

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

why does the test suddenly need so much more time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The consumer tests are rather fast, it is that other tests that use the older mock broker logic (the Returns() plus the default handler to be more specific) sometimes take longer to complete. The thing is that in the original mock broker implementation it would delay a response to a request indefinitely until a reply is provided via Returns() method. Now if a mock broker happens to get a request before Returns() is called it waits for expectationTimeout for Returns() to be called and then ignores the request making it IO timeout on the consumer side. Consumer retries eventually but that adds up to the execution time.

I will try to increase the expectationTimeout and bring the overall test suite timeout to the original value.

@eapache
Copy link
Contributor

eapache commented Aug 6, 2015

A few small points, but otherwise this looks really excellent, thank you!

@horkhe
Copy link
Contributor Author

horkhe commented Aug 6, 2015

@eapache done

@eapache
Copy link
Contributor

eapache commented Aug 6, 2015

👍

@wvanbergen any comments?

@wvanbergen
Copy link
Contributor

This looks good to me! Merging...

wvanbergen added a commit that referenced this pull request Aug 6, 2015
Further mockbrocker improvements
@wvanbergen wvanbergen merged commit 3ef4815 into IBM:master Aug 6, 2015
@horkhe horkhe deleted the maxim/stash branch August 7, 2015 05:23
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.

3 participants