KAFKA-9088: InternalProcessorContext mock builder [partial]

[pierDipi]

I made a simpler version of a builder for mocking InternalProcessorContext.

ref: 7718

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[pierDipi]

Call for review: @cadonna @mjsax @guozhangwang @vvcephei

[guozhangwang]

@cadonna could you take another look?

[cadonna]

@pierDipi Thank you very much for this PR. This is how I envisioned the mock.

I did a first pass without looking too much to the unit tests.

[cadonna]

Q: Do you think we need to implement this setter behaviour for this mock? Couldn't we not just pass a map of state stores at during the build specification with a method like stateStores(final Map<String, StateStore> stateStores) and don't specify a behaviour for register()?

[pierDipi]

Hi @cadonna,
I didn't get the advantage of this.
I would point out that register() is often called here: InMemoryWindowStore.java#L111

Anyway, I made all requested modifications except this one, could you take another look?
Thank you.

[cadonna]

@pierDipi Sorry for the late reply.
Actually, during our discussions, I thought we agreed to stop the EasyMock experiment because of the public API issue and we should try to do something like #7979 as proposed by @vvcephei. Sorry for the confusion.
Anyways, I would be really happy, if you could take #7979 and extend it to make it robust. Are you still interested?

[pierDipi]

@cadonna Yes, I'm still interested, but I cannot guarantee when it will happen. I hope it will be done by the end of the next month.

[vvcephei]

Hi @pierDipi @cadonna ,

Thanks for the follow-up, this looks cleaner than the last PR.

I understand that part of the point here is to explore the EasyMock approach. Given what I said on the last PR, I'm sure it's not surprising if I state that I'm still concerned this is not the best approach.

Because such concerns are often too high-level to be useful, I put together a small PR purely to demonstrate the alternative form this work could take: #7979

A couple of things to note:

1. Just as in this PR, POC: Consolidate testing InternalProcessorContexts #7979 results in an InternalProcessorContext suitable for the remainder of the tests that still depend on the InternalMockProcessorContext
2. Unlike this PR, POC: Consolidate testing InternalProcessorContexts #7979 converts a test (KStreamSessionWindowAggregateProcessorTest) to demonstrate what the testing usage would look like. Perhaps a good comparative study would be to convert the same test as part of this PR.
3. Looking just at the support code (not the test) in POC: Consolidate testing InternalProcessorContexts #7979 only requires 17 lines of code to change from the existing trunk, and there are a total of only 124 lines of code in the mocked InternalProcessorContext as a whole. In comparison, this PR's mock is 376 lines. LOC isn't always the most useful metric for comparison, but the fact that the EasyMock approach requires 3x the testing support code is concerning.
4. POC: Consolidate testing InternalProcessorContexts #7979 allows us to reuse the existing public test-util API MockProcessorContext, which (by dogfooding) increases our confidence and decreases the chance of regression in the public test utility.
5. Unlike the EasyMock approach, POC: Consolidate testing InternalProcessorContexts #7979 is pure, easy-to-read, debuggable and traceable Java.

I really want to underscore that I appreciate all the work you've both put into exploring the EasyMock approach, and that I don't have a particular beef with EasyMock. Just that EasyMock is highest leverage when used in small and localized ways. When you have to put together a builder for a mock, it indicates right away that the use case is not ideal for EasyMock.

[pierDipi]

Hi @cadonna @vvcephei,

Thanks for the reviews.

My first PR merged InternalMockProcessorContext and MockInternalProcessorContext: #7594, which could be a good comparison in terms of LOC, and AFAIU it's the strategy that @vvcephei likes.
Just a note: #7594 changes MockProcessorContext which is a user-facing API, which means the PR requires further improvements.

2. Unlike this PR, POC: Consolidate testing InternalProcessorContexts #7979 converts a test (KStreamSessionWindowAggregateProcessorTest) to demonstrate what the testing usage would look like. Perhaps a good comparative study would be to convert the same test as part of this PR.

Yes, of course.

3. Looking just at the support code (not the test) in POC: Consolidate testing InternalProcessorContexts #7979 only requires 17 lines of code to change from the existing trunk, and there are a total of only 124 lines of code in the mocked InternalProcessorContext as a whole. In comparison, this PR's mock is 376 lines. LOC isn't always the most useful metric for comparison, but the fact that the EasyMock approach requires 3x the testing support code is concerning.

If the comparison is based on LOC to change, IMHO, there is no way we will be able to find a solution based on EasyMock that has less LOC to change; at most, we will be able to find a solution that overall has less LOC, given that we can delete InternalMockProcessorContext and MockInternalProcessorContext.

4. POC: Consolidate testing InternalProcessorContexts #7979 allows us to reuse the existing public test-util API MockProcessorContext, which (by dogfooding) increases our confidence and decreases the chance of regression in the public test utility.

The builder uses it too.

5. Unlike the EasyMock approach, POC: Consolidate testing InternalProcessorContexts #7979 is pure, easy-to-read, debuggable and traceable Java.

This is a good point.

[cadonna]

@vvcephei and @pierDipi Thanks for your comments. I will try to answer to your comments as soon as possible.

[cadonna]

First of all thank you very much for the discussion, @pierDipi and @vvcephei. I have now a much clearer picture about the implications of using EasyMock in this case.

My original idea was to try to use EasyMock to mock InternalProcessorContext to let EasyMock handle call verification and get rid of our own call verification code. The goal was to have less code to maintain.

@vvcephei, while I also think we should try to avoid unnecessary complexity, I also find that your LOC comparison is not completely fair for the following reasons:

• The solution with EasyMock gives you the possibility to add call verification on any public method you want which PR POC: Consolidate testing InternalProcessorContexts #7979 (nice PR number, BTW) does not give you.
• As I wrote above, the goal was to have less code to maintain, i.e., we would need to also include the LOC that could be removed because of this PR in the calculation.

Regarding readability, that would still need a couple of iterations of this PR. I cannot follow your reasoning about debuggability and traceability. While I see why it is important to debug a hand-crafted mock, I do not see why it is important to debug a mock created by an external library. Admittedly, there are for sure cases where this is needed, but they should be rare.

Said that let's now move to my concerns that arose during our discussions. To really accomplish the goal to let EasyMock handle all call verifications and not introducing too much complexity, we would need to refactor also MockProcessorContext and to use an EasyMock also for it, because in that class resided the majority of self-made call verification code that EasyMock should handle. That would also mean that we would make our public API (MockProcessorContext is part of the public test-utils and there are plans also to move a mock for InternalProcessorContext to test-utils) dependent on EasyMock. IMO that is a big issue, because we would limit our freedom in changing the API and make the API in general more brittle. For this reason, I think EasyMock is not the right approach in this case.

What do you think?

@pierDipi
Thank you very much for all your work to get to this point. I am sorry that I overlooked this fact in the beginning. It would have saved you some work. If no public API were involved, I would opt to go ahead with the EasyMock approach. I hope you are not too much disappointed about how this work developed and that you will continue with this ticket.

Again, thank you both for the discussions. I learned a lot.

[vvcephei]

Thanks @pierDipi and @cadonna ,

Just to clarify, although the LOC point may be invalid, it sounds like you basically in agreement that (at least for now), we can do something more like #7979 than an EasyMock builder. If so, then I'm (obviously) +1.

As a side note, I do think that we should consider what is right for each test. For example, if the desire is just to provide a "dummy" context and verify the black-box behavior of a component (such as, "does this processor forward the right result for the given inputs?"), then the #7979 approach is preferable. However, if the goal is really to verify some specific interaction (like, "does this method get called exactly twice?"), then a mock can still be defined in situ, which we do in many tests mocking other components. IMHO, this strategy plays to the strengths of both approaches.

Thanks again,
-John

[guozhangwang]

test this please

[guozhangwang]

@pierDipi @cadonna @vvcephei could you bring me up to date on the current progress of this PR?

[pierDipi]

Hi @guozhangwang,

We decided to go ahead with #7979 and therefore this PR can be closed.