kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From John Roesler <j...@confluent.io>
Subject Re: [DISCUSS] KIP-267: Add Processor Unit Test Support to Kafka Streams Test Utils
Date Fri, 09 Mar 2018 04:51:40 GMT
I think what you're suggesting is to:
1. compile the main streams code, but not the tests
2. compile test-utils (and compile and run the test-utils tests)
3. compile and run the streams tests

This works in theory, since the test-utils depends on the main streams
code, but not the streams tests. and the streams tests depend on test-utils
while the main streams code does not.

But after poking around a bit and reading up on it, I think this is not
possible, or at least not mainstream.

The issue is that dependencies are formed between projects, in this case
streams and streams:test-utils. The upstream project must be built before
the dependant one, regardless of whether the dependency is for compiling
the main code or the test code. This means we do have a circular dependency
on our hands if we want the tests in streams to use the test-utils, since
they'd both have to be built before the other.

Gradle seems to be quite scriptable, so there may be some way to achieve
this, but increasing the complexity of the build also introduces a project
maintenance concern.

The MockProcessorContext itself is pretty simple, so I'm tempted to argue
that we should just have one for internal unit tests and another for
test-utils, however this situation also afflicts KAFKA-6474
<https://issues.apache.org/jira/browse/KAFKA-6474>, and the
TopologyTestDriver is not so trivial.

I think the best thing at this point is to go ahead and fold the test-utils
into the streams project. We can put it into a separate "testutils" package
to make it easy to identify which code is for test support and which code
is Kafka Streams. The biggest bummer about this suggestion is that it we
*just* introduced the test-utils artifact, so folks would to add that
artifact in 1.1 to write their tests and then have to drop it again in 1.2.

The other major solution is to create a new gradle project for the streams
unit tests, which depends on streams and test-utils and move all the
streams unit tests there. I'm pretty sure we can configure gradle just to
include this project for running tests and not actually package any
artifacts. This structure basically expresses your observation that the
test code is essentially a separate module from the main streams code.

Of course, I'm open to alternatives, especially if someone with more
experience in Gradle is aware of a solution.

Thanks,
-John


On Thu, Mar 8, 2018 at 3:39 PM, Matthias J. Sax <matthias@confluent.io>
wrote:

> Isn't MockProcessorContext in o.a.k.test part of the unit-test package
> but not the main package?
>
> This should resolve the dependency issue.
>
> -Matthias
>
> On 3/8/18 3:32 PM, John Roesler 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 <john@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 <wangguoz@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 <john@confluent.io>
> wrote:
> >>>
> >>>> On Wed, Mar 7, 2018 at 8:03 PM, John Roesler <john@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 <yuzhihong@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 <john@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
> >>>
> >>
> >>
> >
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message