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 >> > >
