MockProcessorContext is only used in unit tests, and hence we should be able to declare it as a test dependency of `streams` in gradle build file, which is OK.
Guozhang On Thu, Mar 8, 2018 at 3:32 PM, John Roesler <j...@confluent.io> wrote: > Actually, replacing the MockProcessorContext in o.a.k.test could be a bit > tricky, since it would make the "streams" module depend on > "streams:test-utils", but "streams:test-utils" already depends on > "streams". > > At first glance, it seems like the options are: > 1. leave the two separate implementations in place. This shouldn't be > underestimated, especially since our internal tests may need different > things from a mocked P.C. than our API users. > 2. move the public testing artifacts into the regular streams module > 3. move the unit tests for Streams into a third module that depends on both > streams and test-utils. Yuck! > > Thanks, > -John > > On Thu, Mar 8, 2018 at 3:16 PM, John Roesler <j...@confluent.io> wrote: > > > Thanks for the review, Guozhang, > > > > In response: > > 1. I missed that! I'll look into it and update the KIP. > > > > 2. I was planning to use the real implementation, since folks might > > register some metrics in the processors and want to verify the values > that > > get recorded. If the concern is about initializing all the stuff that's > in > > the Metrics object, I can instantiate it lazily or even make it optional > by > > taking a nullable constructor parameter. > > > > 3. Agreed. I think that's the real sharp edge here. I actually think it > > would be neat to auto-trigger those scheduled punctuators, but it seems > > like that moves this component out of "mock" territory and into "driver" > > territory. Since we already have the TopologyTestDriver, I'd prefer to > > focus on keeping the mock lean. I agree it should be in the javadoc as > well > > as the web documentation. > > > > Thanks, > > -John > > > > On Thu, Mar 8, 2018 at 1:46 PM, Guozhang Wang <wangg...@gmail.com> > wrote: > > > >> Hello John, > >> > >> Thanks for the KIP. I made a pass over the wiki page and here are some > >> comments: > >> > >> 1. Meta-comment: there is an internal class MockProcessorContext under > the > >> o.a.k.test package, which should be replaced as part of this KIP. > >> > >> 2. In @Override StreamsMetrics metrics(), will you return a fully > created > >> StreamsMetricsImpl object or are you planning to use the > >> MockStreamsMetrics? Note that for the latter case you probably need to > >> look > >> into https://issues.apache.org/jira/browse/KAFKA-5676 as well. > >> > >> 3. Not related to the KIP changes themselves: about > >> "context.scheduledPunctuators": we need to well document that in the > >> MockProcessorContext the scheduled punctuator will never by > >> auto-triggered, > >> and hence it is only for testing people's code that some punctuators are > >> indeed registered, and if people want full auto punctuation testing they > >> have to go with TopologyTestDriver. > >> > >> > >> > >> Guozhang > >> > >> > >> On Wed, Mar 7, 2018 at 8:04 PM, John Roesler <j...@confluent.io> wrote: > >> > >> > On Wed, Mar 7, 2018 at 8:03 PM, John Roesler <j...@confluent.io> > wrote: > >> > > >> > > Thanks Ted, > >> > > > >> > > Sure thing; I updated the example code in the KIP with a little > >> snippet. > >> > > > >> > > -John > >> > > > >> > > On Wed, Mar 7, 2018 at 7:18 PM, Ted Yu <yuzhih...@gmail.com> wrote: > >> > > > >> > >> Looks good. > >> > >> > >> > >> See if you can add punctuator into the sample code. > >> > >> > >> > >> On Wed, Mar 7, 2018 at 7:10 PM, John Roesler <j...@confluent.io> > >> wrote: > >> > >> > >> > >> > Dear Kafka community, > >> > >> > > >> > >> > I am proposing KIP-267 to augment the public Streams test utils > >> API. > >> > >> > The goal is to simplify testing of Kafka Streams applications. > >> > >> > > >> > >> > Please find details in the > >> > >> > wiki:https://cwiki.apache.org/confluence/display/KAFKA/KIP- > >> > >> > 267%3A+Add+Processor+Unit+Test+Support+to+Kafka+Streams+ > Test+Utils > >> > >> > > >> > >> > An initial WIP PR can be found here:https://github.com/ > >> > >> > apache/kafka/pull/4662 > >> > >> > > >> > >> > I also included the user-list (please hit "reply-all" to include > >> both > >> > >> > lists in this KIP discussion). > >> > >> > > >> > >> > Thanks, > >> > >> > > >> > >> > -John > >> > >> > > >> > >> > >> > > > >> > > > >> > > >> > >> > >> > >> -- > >> -- Guozhang > >> > > > > > -- -- Guozhang
