[jira] [Commented] (KAFKA-8977) Remove MockStreamsMetrics Since it is not a Mock
[
https://issues.apache.org/jira/browse/KAFKA-8977?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17738499#comment-17738499
]
Joobi S B commented on KAFKA-8977:
--
Hi [~cadonna] , since this was not picked up i've worked on it and have raised
a PR, could you please have a look https://github.com/apache/kafka/pull/13931
> Remove MockStreamsMetrics Since it is not a Mock
>
>
> Key: KAFKA-8977
> URL: https://issues.apache.org/jira/browse/KAFKA-8977
> Project: Kafka
> Issue Type: Improvement
> Components: streams
>Reporter: Bruno Cadonna
>Assignee: Joobi S B
>Priority: Minor
> Labels: newbie
>
> The class {{MockStreamsMetrics}} is used throughout unit tests as a mock but
> it is not really a mock since it only hides two parameters of the
> {{StreamsMetricsImpl}} constructor. Either a real mock or the real
> {{StreamsMetricsImpl}} should be used in the tests.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (KAFKA-8977) Remove MockStreamsMetrics Since it is not a Mock
[
https://issues.apache.org/jira/browse/KAFKA-8977?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16973497#comment-16973497
]
bibin sebastian commented on KAFKA-8977:
[~cadonna] not sure if you got a chance to look at my PR.
> Remove MockStreamsMetrics Since it is not a Mock
>
>
> Key: KAFKA-8977
> URL: https://issues.apache.org/jira/browse/KAFKA-8977
> Project: Kafka
> Issue Type: Improvement
> Components: streams
>Reporter: Bruno Cadonna
>Assignee: bibin sebastian
>Priority: Minor
> Labels: newbie
>
> The class {{MockStreamsMetrics}} is used throughout unit tests as a mock but
> it is not really a mock since it only hides two parameters of the
> {{StreamsMetricsImpl}} constructor. Either a real mock or the real
> {{StreamsMetricsImpl}} should be used in the tests.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (KAFKA-8977) Remove MockStreamsMetrics Since it is not a Mock
[
https://issues.apache.org/jira/browse/KAFKA-8977?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16970811#comment-16970811
]
bibin sebastian commented on KAFKA-8977:
[~cadonna] ^^
> Remove MockStreamsMetrics Since it is not a Mock
>
>
> Key: KAFKA-8977
> URL: https://issues.apache.org/jira/browse/KAFKA-8977
> Project: Kafka
> Issue Type: Improvement
> Components: streams
>Reporter: Bruno Cadonna
>Assignee: bibin sebastian
>Priority: Minor
> Labels: newbie
>
> The class {{MockStreamsMetrics}} is used throughout unit tests as a mock but
> it is not really a mock since it only hides two parameters of the
> {{StreamsMetricsImpl}} constructor. Either a real mock or the real
> {{StreamsMetricsImpl}} should be used in the tests.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (KAFKA-8977) Remove MockStreamsMetrics Since it is not a Mock
[
https://issues.apache.org/jira/browse/KAFKA-8977?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16967683#comment-16967683
]
bibin sebastian commented on KAFKA-8977:
[~cadonna] do you think that my PR above is up for review?
> Remove MockStreamsMetrics Since it is not a Mock
>
>
> Key: KAFKA-8977
> URL: https://issues.apache.org/jira/browse/KAFKA-8977
> Project: Kafka
> Issue Type: Improvement
> Components: streams
>Reporter: Bruno Cadonna
>Assignee: bibin sebastian
>Priority: Minor
> Labels: newbie
>
> The class {{MockStreamsMetrics}} is used throughout unit tests as a mock but
> it is not really a mock since it only hides two parameters of the
> {{StreamsMetricsImpl}} constructor. Either a real mock or the real
> {{StreamsMetricsImpl}} should be used in the tests.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (KAFKA-8977) Remove MockStreamsMetrics Since it is not a Mock
[
https://issues.apache.org/jira/browse/KAFKA-8977?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16965595#comment-16965595
]
bibin sebastian commented on KAFKA-8977:
Ok, I get your point.
Yes, I would like to remove {{MockStreamsMetrics}} with {{StreamsMetricsImpl
for now as the first step.}}
> Remove MockStreamsMetrics Since it is not a Mock
>
>
> Key: KAFKA-8977
> URL: https://issues.apache.org/jira/browse/KAFKA-8977
> Project: Kafka
> Issue Type: Improvement
> Components: streams
>Reporter: Bruno Cadonna
>Assignee: bibin sebastian
>Priority: Minor
> Labels: newbie
>
> The class {{MockStreamsMetrics}} is used throughout unit tests as a mock but
> it is not really a mock since it only hides two parameters of the
> {{StreamsMetricsImpl}} constructor. Either a real mock or the real
> {{StreamsMetricsImpl}} should be used in the tests.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (KAFKA-8977) Remove MockStreamsMetrics Since it is not a Mock
[
https://issues.apache.org/jira/browse/KAFKA-8977?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16964717#comment-16964717
]
Bruno Cadonna commented on KAFKA-8977:
--
I agree with you that mocks are often just used to mock direct dependencies and
verify their usage. However, mocks can also be used to just return values for
the methods of the mocked object without any verification. That is also how
`MockStreamsMetrics` is used at the moment, except that it uses production code
to compute the return values. I would like to have mock that just returns
constants. Maybe for some methods, we will need to provide some functionality
to set the returned values during test setup. With this I would like to achieve
a better isolation of unit tests. That is, I do not want that a unit test that
uses `StreamsMetricsImpl` fails because there is a bug in `StreamsMetricsImpl`.
Only the tests in `StreamsMetricsImplTest` should fail in that case. The goal
is to reduce the maintenance costs of unit tests.
If you just want to remove {{MockStreamsMetrics}} with {{StreamsMetricsImpl}},
that is fine with me. IMO, this is already a step in the right direction.
> Remove MockStreamsMetrics Since it is not a Mock
>
>
> Key: KAFKA-8977
> URL: https://issues.apache.org/jira/browse/KAFKA-8977
> Project: Kafka
> Issue Type: Improvement
> Components: streams
>Reporter: Bruno Cadonna
>Assignee: bibin sebastian
>Priority: Minor
> Labels: newbie
>
> The class {{MockStreamsMetrics}} is used throughout unit tests as a mock but
> it is not really a mock since it only hides two parameters of the
> {{StreamsMetricsImpl}} constructor. Either a real mock or the real
> {{StreamsMetricsImpl}} should be used in the tests.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (KAFKA-8977) Remove MockStreamsMetrics Since it is not a Mock
[
https://issues.apache.org/jira/browse/KAFKA-8977?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16964669#comment-16964669
]
bibin sebastian commented on KAFKA-8977:
I am assuming you are suggesting to create a generic mock of
`StreamsMetricsImpl` using the PowerMock/EasyMock and externalise for all
tests. Technically I think that is possible.
(Actually I think that is the only way to create mock for `StreamsMetricsImpl`.
Creating the mock by implementing the interface `StreamsMetrics` won't really
work because in most of the places `StreamsMetricsImpl` is directly referred
instead of using the interface references. Also this class have multiple static
methods. So I have a feeling that this class is used like a util class.)
So going by your suggestion, even if I create a generic mock, I am still not
convinced on the value of creating a mock in these scenarios where we have
indirect access to `StreamsMetricsImpl`.
Let me try to explain my thought process here.
Generally I use a mock instance to mock any external (direct) dependencies of
the class/method in test and I verify the usage of mock as part of assertion in
the end. In our case since StreamsMetricsImpl is not being directly referred
(most of the time) in classes/methods in test, even if we create a mock we
won't be realistically be able to verify the usage (methods invocations) on the
mock (using verify(mock) method). This is because we wouldn't want our tests to
fail assertion if the indirect usage of StreamsMetricsImpl changes at a later
point of time, so verifying the mock usage in the assertion wont really make
sense. So my point is what is the real value we gain if we can't verify the
usage of mock?
So I was preferring to leave its usage as it is in its full form rather than
using a mock. What do think?
[~cadonna]
> Remove MockStreamsMetrics Since it is not a Mock
>
>
> Key: KAFKA-8977
> URL: https://issues.apache.org/jira/browse/KAFKA-8977
> Project: Kafka
> Issue Type: Improvement
> Components: streams
>Reporter: Bruno Cadonna
>Assignee: bibin sebastian
>Priority: Minor
> Labels: newbie
>
> The class {{MockStreamsMetrics}} is used throughout unit tests as a mock but
> it is not really a mock since it only hides two parameters of the
> {{StreamsMetricsImpl}} constructor. Either a real mock or the real
> {{StreamsMetricsImpl}} should be used in the tests.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (KAFKA-8977) Remove MockStreamsMetrics Since it is not a Mock
[
https://issues.apache.org/jira/browse/KAFKA-8977?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16961829#comment-16961829
]
Bruno Cadonna commented on KAFKA-8977:
--
I see your point. Isn't it possible to come up with a common setup for a
`StreamsMetricsImpl` mock and externalise it, so that this mock can be reused
in multiple tests?
> Remove MockStreamsMetrics Since it is not a Mock
>
>
> Key: KAFKA-8977
> URL: https://issues.apache.org/jira/browse/KAFKA-8977
> Project: Kafka
> Issue Type: Improvement
> Components: streams
>Reporter: Bruno Cadonna
>Assignee: bibin sebastian
>Priority: Minor
> Labels: newbie
>
> The class {{MockStreamsMetrics}} is used throughout unit tests as a mock but
> it is not really a mock since it only hides two parameters of the
> {{StreamsMetricsImpl}} constructor. Either a real mock or the real
> {{StreamsMetricsImpl}} should be used in the tests.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (KAFKA-8977) Remove MockStreamsMetrics Since it is not a Mock
[
https://issues.apache.org/jira/browse/KAFKA-8977?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16961278#comment-16961278
]
bibin sebastian commented on KAFKA-8977:
[~cadonna] Having going through all the usages of existing MockStreamsMetrics,
what I generally see is that MockStreamsMetrics is used during initialisation
steps of the test (For example to create ThreadCache and session store). The
tests are not always directly accessing/invoking the methods of StreamsMetrics.
Almost all the times access is indirect (As mentioned in the previos above
observation as well). So I am not really seeing any great the value in creating
a mock on StreamsMetricsImpl via PowerMock/EasyMock and replacing
MockStreamsMetrics.
Actually at first, I started with creating the mock using PowerMock/EasyMock.
First of all it forced me to look at the initialisation steps and mock most of
the methods for the initialisation steps. On top of these, since these tests
are indirectly using StreamsMetricsImpl, I have to create separate expectation
on mocks just to make the test pass.
Had it been StreamsMetricsImpl being directly referred in the test methods, it
would have been easy to mock the relevant methods but that is not the case in
almost all the cases.
All these findings makes me feel that replacing MockStreamsMetrics with
StreamsMetricsImpl is the easiest and cleanest option at the moment. In some
simple cases, I have used niceMock().
Updated the PR above. Can you please take a look?
> Remove MockStreamsMetrics Since it is not a Mock
>
>
> Key: KAFKA-8977
> URL: https://issues.apache.org/jira/browse/KAFKA-8977
> Project: Kafka
> Issue Type: Improvement
> Components: streams
>Reporter: Bruno Cadonna
>Assignee: bibin sebastian
>Priority: Minor
> Labels: newbie
>
> The class {{MockStreamsMetrics}} is used throughout unit tests as a mock but
> it is not really a mock since it only hides two parameters of the
> {{StreamsMetricsImpl}} constructor. Either a real mock or the real
> {{StreamsMetricsImpl}} should be used in the tests.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (KAFKA-8977) Remove MockStreamsMetrics Since it is not a Mock
[
https://issues.apache.org/jira/browse/KAFKA-8977?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16960981#comment-16960981
]
Bruno Cadonna commented on KAFKA-8977:
--
[~bibin84] Sorry for the late reply.
Sounds good to me, looking forward to review your PR.
> Remove MockStreamsMetrics Since it is not a Mock
>
>
> Key: KAFKA-8977
> URL: https://issues.apache.org/jira/browse/KAFKA-8977
> Project: Kafka
> Issue Type: Improvement
> Components: streams
>Reporter: Bruno Cadonna
>Assignee: bibin sebastian
>Priority: Minor
> Labels: newbie
>
> The class {{MockStreamsMetrics}} is used throughout unit tests as a mock but
> it is not really a mock since it only hides two parameters of the
> {{StreamsMetricsImpl}} constructor. Either a real mock or the real
> {{StreamsMetricsImpl}} should be used in the tests.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (KAFKA-8977) Remove MockStreamsMetrics Since it is not a Mock
[
https://issues.apache.org/jira/browse/KAFKA-8977?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16960920#comment-16960920
]
ASF GitHub Bot commented on KAFKA-8977:
---
bibinss commented on pull request #7603: KAFKA-8977: Remove MockStreamsMetrics
Since it is not a Mock
URL: https://github.com/apache/kafka/pull/7603
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> Remove MockStreamsMetrics Since it is not a Mock
>
>
> Key: KAFKA-8977
> URL: https://issues.apache.org/jira/browse/KAFKA-8977
> Project: Kafka
> Issue Type: Improvement
> Components: streams
>Reporter: Bruno Cadonna
>Assignee: bibin sebastian
>Priority: Minor
> Labels: newbie
>
> The class {{MockStreamsMetrics}} is used throughout unit tests as a mock but
> it is not really a mock since it only hides two parameters of the
> {{StreamsMetricsImpl}} constructor. Either a real mock or the real
> {{StreamsMetricsImpl}} should be used in the tests.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (KAFKA-8977) Remove MockStreamsMetrics Since it is not a Mock
[
https://issues.apache.org/jira/browse/KAFKA-8977?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16951855#comment-16951855
]
bibin sebastian commented on KAFKA-8977:
Got it. Glancing through the test cases/classes where MockStreamsMetrics is
used, what I see is that in lot of the test cases, class/object that is tested,
already have references to many other classes. streamsMetrics instance is
passed down to these reference classes and they in turn call the methods on
streamsMetrics.
Here is an example. ThreadCache accepts StreamsMetrics as a constructor
parameter (this.metrics) and this is passed down to NamesCache instance as
follows in getOrCreateCache() method.
{code:java}
public ThreadCache(final LogContext logContext, final long maxCacheSizeBytes,
final StreamsMetricsImpl metrics) {
this.maxCacheSizeBytes = maxCacheSizeBytes;
this.metrics = metrics;
this.log = logContext.logger(getClass());
}
private synchronized NamedCache getOrCreateCache(final String name) {
NamedCache cache = caches.get(name);
if (cache == null) {
cache = new NamedCache(name, this.metrics);
caches.put(name, cache);
}
return cache;
}
{code}
Now if you look at the constructor of NamedCache (shown below), streamsMetrics
is again passed down to NamedCacheMetrics.
{code:java}
NamedCache(final String name, final StreamsMetricsImpl streamsMetrics) {
this.name = name;
this.streamsMetrics = streamsMetrics;
storeName = ThreadCache.underlyingStoreNamefromCacheName(name);
taskName = ThreadCache.taskIDfromCacheName(name);
hitRatioSensor = NamedCacheMetrics.hitRatioSensor(
streamsMetrics,
Thread.currentThread().getName(),
taskName,
storeName
);
}
{code}
While replacing MockStreamsMetrics with an EasyMock/PowerMock instances, I am
also trying to mock the external dependencies (wherever appropriate) to reduce
the scope of the mocking StreamsMetricsImpl. So in this just above mentioned
case I will also mock NamedCacheMetrics.hitRatioSensor() as this is an external
logic to ThreadCache. IMO this will leave tests in a better shape as a general
practice it is good to mock all the external dependencies in a unit test.
There are many test files where we have this above mentioned situation. I will
share the PR once I am done with the changes.
> Remove MockStreamsMetrics Since it is not a Mock
>
>
> Key: KAFKA-8977
> URL: https://issues.apache.org/jira/browse/KAFKA-8977
> Project: Kafka
> Issue Type: Improvement
> Components: streams
>Reporter: Bruno Cadonna
>Assignee: bibin sebastian
>Priority: Minor
> Labels: newbie
>
> The class {{MockStreamsMetrics}} is used throughout unit tests as a mock but
> it is not really a mock since it only hides two parameters of the
> {{StreamsMetricsImpl}} constructor. Either a real mock or the real
> {{StreamsMetricsImpl}} should be used in the tests.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (KAFKA-8977) Remove MockStreamsMetrics Since it is not a Mock
[
https://issues.apache.org/jira/browse/KAFKA-8977?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16951857#comment-16951857
]
bibin sebastian commented on KAFKA-8977:
[~cadonna]^^
> Remove MockStreamsMetrics Since it is not a Mock
>
>
> Key: KAFKA-8977
> URL: https://issues.apache.org/jira/browse/KAFKA-8977
> Project: Kafka
> Issue Type: Improvement
> Components: streams
>Reporter: Bruno Cadonna
>Assignee: bibin sebastian
>Priority: Minor
> Labels: newbie
>
> The class {{MockStreamsMetrics}} is used throughout unit tests as a mock but
> it is not really a mock since it only hides two parameters of the
> {{StreamsMetricsImpl}} constructor. Either a real mock or the real
> {{StreamsMetricsImpl}} should be used in the tests.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (KAFKA-8977) Remove MockStreamsMetrics Since it is not a Mock
[
https://issues.apache.org/jira/browse/KAFKA-8977?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16945729#comment-16945729
]
Bruno Cadonna commented on KAFKA-8977:
--
Thank you for picking this up.
IMO we should mock {{StreamsMetricsImpl}} in the test where we do not directly
test {{StreamsMetricsImpl}}. However, there is no need to write a mock. I would
use {{EasyMock}} or {{PowerMock}} to mock {{StreamsMetricsImpl}}. You would use
{{EasyMock}} if only instance methods need to be mocked and {{PowerMock}} if
class methods or final methods need to be mocked. You can find an example of
both types of mocks for {{StreamsMetricsImpl}} in {{NamedCacheMetricsTest}}.
> Remove MockStreamsMetrics Since it is not a Mock
>
>
> Key: KAFKA-8977
> URL: https://issues.apache.org/jira/browse/KAFKA-8977
> Project: Kafka
> Issue Type: Improvement
> Components: streams
>Reporter: Bruno Cadonna
>Assignee: bibin sebastian
>Priority: Minor
> Labels: newbie
>
> The class {{MockStreamsMetrics}} is used throughout unit tests as a mock but
> it is not really a mock since it only hides two parameters of the
> {{StreamsMetricsImpl}} constructor. Either a real mock or the real
> {{StreamsMetricsImpl}} should be used in the tests.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (KAFKA-8977) Remove MockStreamsMetrics Since it is not a Mock
[
https://issues.apache.org/jira/browse/KAFKA-8977?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16945258#comment-16945258
]
bibin sebastian commented on KAFKA-8977:
[~cadonna] shall I remove MockStreamsMetrics and instead use StreamsMetricsImpl
in the testcases?
I dont see a real need to create a mock for StreamsMetricsImpl now.
> Remove MockStreamsMetrics Since it is not a Mock
>
>
> Key: KAFKA-8977
> URL: https://issues.apache.org/jira/browse/KAFKA-8977
> Project: Kafka
> Issue Type: Improvement
> Components: streams
>Reporter: Bruno Cadonna
>Assignee: bibin sebastian
>Priority: Minor
> Labels: newbie
>
> The class {{MockStreamsMetrics}} is used throughout unit tests as a mock but
> it is not really a mock since it only hides two parameters of the
> {{StreamsMetricsImpl}} constructor. Either a real mock or the real
> {{StreamsMetricsImpl}} should be used in the tests.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
