chia7712 commented on code in PR #15488:
URL: https://github.com/apache/kafka/pull/15488#discussion_r1518695740
##########
core/src/test/scala/unit/kafka/log/LogLoaderTest.scala:
##########
@@ -352,19 +352,15 @@ class LogLoaderTest {
// Intercept all segment read calls
val interceptedLogSegments = new LogSegments(topicPartition) {
override def add(segment: LogSegment): LogSegment = {
- val wrapper = new LogSegment(segment) {
-
- override def read(startOffset: Long, maxSize: Int, maxPosition:
Long, minOneMessage: Boolean): FetchDataInfo = {
- segmentsWithReads += this
- super.read(startOffset, maxSize, maxPosition, minOneMessage)
- }
-
- override def recover(producerStateManager: ProducerStateManager,
- leaderEpochCache:
Optional[LeaderEpochFileCache]): Int = {
- recoveredSegments += this
- super.recover(producerStateManager, leaderEpochCache)
- }
- }
+ val wrapper = Mockito.spy(segment)
Review Comment:
> There is a lot of magic to make mocks work and even more so for spy mocks.
Replacing readable/debuggable normal code for mocks is generally a bad idea. A
concrete example or the problems with mocks are the mock leaks that have caused
serious test instability.
As a general rule, mocks should only be used if the mock free alternative is
clearly worse in terms of verbosity, maintainability, etc. I don't think it's
the case here.
Thanks for sharing. I agree that `spy` is a black box and it could have
caused serious test instability.
get back to the subject. There are alternatives which can remove the
incomplete copy constructor.
1. add public getters to `LogSegment` in order to reuse the constructor
(https://github.com/apache/kafka/blob/trunk/storage/src/main/java/org/apache/kafka/storage/internals/log/LogSegment.java#L112).
The side-effect is that the testing-only code is added. However, that is
better than having an incomplete copy constructor.
2. reuse the constructor with temporary objects. The is the initial approach
of this PR. The disadvantage is that it needs to create weird objects, but it
is fine for the tests.
I prefer option 2 to replace the `spy`. WDYT?
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
