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

Mockbroker #51

Merged
merged 3 commits into from
Nov 26, 2013
Merged

Mockbroker #51

merged 3 commits into from
Nov 26, 2013

Conversation

burke
Copy link
Contributor

@burke burke commented Nov 12, 2013

Semantics of the tests are identical (AFAIK), but I added an abstraction to generate trivial responses.

It's a bit duplicated from the main package, but there were only a few bad options here:

  1. Expose a bunch of internals;

  2. Link a bunch of testing API bits into the library;

  3. Duplicate some code to generate requests.

Anyway, the major win here is that other apps can import mockbroker to test against mock brokers the same way sarama now does. This is something a bunch of different projects have been struggling with here.

@eapache @fw42 @wvanbergen

@eapache
Copy link
Contributor

eapache commented Nov 12, 2013

I take it 1) refers to the encoder and decoder interfaces and their implementations?

@@ -2,6 +2,7 @@
*.o
*.a
*.so
*.test
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compiled test files. Go creates them sometimes for reasons I'm not especially clear on.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've never seen them. You can pass a flag to go test to make it leave them behind, but in normal case it does everything in /tmp so I'm not sure why these would show up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not too sure. Anyway, they're sometimes created on some machines, so no reason not to gitignore them.

@burke
Copy link
Contributor Author

burke commented Nov 12, 2013

Yeah, the encoders in particular. It seemed minimally horrible to do it this way, though I didn't really think very hard about it.

@eapache
Copy link
Contributor

eapache commented Nov 12, 2013

I am trying to figure out a way to split the encoder/decoder into a separate package that can be shared, and now I am wishing I didn't give up the original debate about go package subdirectories...

@eapache
Copy link
Contributor

eapache commented Nov 12, 2013

And now I'm starting to think it's best to bake it in. The only arguments against are performance and complexity. I can't imagine it realistically affects performance at all; the optimizer might even strip out the extra code when it isn't used.

Regarding complexity, I'm coming to the conclusion that the complexity caused in the API is more than offset by the complexity removed in the code. For example, all of the *_expectation.go files can be removed outright and replaced by a single MockBroker.Expect(encoder). This actually reduces overall API complexity as well, if you count both the test API and the production API.

This is something a bunch of different projects have been struggling with here.

I think this is telling - it sounds like the majority of apps using sarama have been struggling to test themselves, which suggests to me that the mock API should be part of the main sarama API for the same reason go requires the *_test.go files to be in the same directory, not in a test/ subdirectory. 'tis The Go Way.

I fully expect this to be a controversial opinion :) Am I missing any arguments against? Are there compelling arguments for another approach?

This was referenced Nov 14, 2013
@burke burke merged commit 3760827 into master Nov 26, 2013
@eapache eapache deleted the mockbroker branch June 23, 2014 23:39
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