----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35615/#review91575 -----------------------------------------------------------
Some general comments: 1. Regarding the @Before and @After annotations, one suggestion from the JIRA was that we remove any annotations other than "@Test" itself but use scalatest features (for example, http://doc.scalatest.org/2.2.4/#org.scalatest.BeforeAndAfter) instead. Now I cannot remember a strong motiviation for this move, so I feel it may be also OK as you chose to use the junit tags anyways. 2. Regarding org.junit.Assert and org.scalatest.Assertions in imports, if we decide to be junit-heavy instead of scalatest-heavy for our unit tests, we should then use the former for most of the time and only the latter for intercept[..] since it is not supported in the fomer. There seems a few places where both of them are used for fail / assert, etc. core/src/test/scala/integration/kafka/api/ConsumerTest.scala (line 33) <https://reviews.apache.org/r/35615/#comment145012> Seems this import is not used inside the class? core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala (line 22) <https://reviews.apache.org/r/35615/#comment145015> Seems this import is not used as well. BTW I have another general comment regarding this issue. core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala (lines 263 - 266) <https://reviews.apache.org/r/35615/#comment145013> Is this intentional, as we already import org.junit.Assert._? core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala (lines 289 - 294) <https://reviews.apache.org/r/35615/#comment145014> Same as above. core/src/test/scala/integration/kafka/api/ProducerSendTest.scala (line 34) <https://reviews.apache.org/r/35615/#comment145016> Sometimes we use org.scalatest.Assertions.fail() and sometimes we use org.junit.Assert.fail(); it would better that we are consistent in one of them, and personally I recommend following the org.junit package since it is more general. core/src/test/scala/unit/kafka/admin/AdminTest.scala (lines 29 - 30) <https://reviews.apache.org/r/35615/#comment145017> Since we already imported Assert._ we do not need to import the other Assertions._ for this class. core/src/test/scala/unit/kafka/network/SocketServerTest.scala (line 35) <https://reviews.apache.org/r/35615/#comment145026> Shall we import import org.junit.{After, Before, Test} instead of org.scalatest.junit.JUnitSuite? core/src/test/scala/unit/kafka/producer/ProducerTest.scala (lines 26 - 34) <https://reviews.apache.org/r/35615/#comment145027> Could you group the imports of org.*, java.*, kafka.*, etc together? Same for some other places. core/src/test/scala/unit/kafka/producer/ProducerTest.scala (line 118) <https://reviews.apache.org/r/35615/#comment145028> I think we can just use org.junit.Assert.fail here. core/src/test/scala/unit/kafka/producer/ProducerTest.scala (line 131) <https://reviews.apache.org/r/35615/#comment145029> same above. core/src/test/scala/unit/kafka/producer/ProducerTest.scala (line 144) <https://reviews.apache.org/r/35615/#comment145030> same above. core/src/test/scala/unit/kafka/producer/ProducerTest.scala (line 203) <https://reviews.apache.org/r/35615/#comment145031> same above. core/src/test/scala/unit/kafka/producer/ProducerTest.scala (line 263) <https://reviews.apache.org/r/35615/#comment145032> same above core/src/test/scala/unit/kafka/producer/ProducerTest.scala (line 297) <https://reviews.apache.org/r/35615/#comment145033> same above core/src/test/scala/unit/kafka/producer/ProducerTest.scala (line 311) <https://reviews.apache.org/r/35615/#comment145034> same above - Guozhang Wang On June 18, 2015, 6:53 p.m., Alexander Pakulov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35615/ > ----------------------------------------------------------- > > (Updated June 18, 2015, 6:53 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1782 > https://issues.apache.org/jira/browse/KAFKA-1782 > > > Repository: kafka > > > Description > ------- > > KAFKA-1782; Junit3 Misusage > > > Diffs > ----- > > core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala > f56096b826bdbf760411a54ba067a6a83eca8a10 > core/src/test/scala/integration/kafka/api/ConsumerTest.scala > 17b17b9b1520c7cc2e2ba96cdb1f9ff06e47bcad > core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala > 07b1ff47bfc3cd3f948c9533c8dc977fa36d996f > core/src/test/scala/integration/kafka/api/ProducerBounceTest.scala > ce70a0a449883723a9b59ea48da34ba30b3f6daf > core/src/test/scala/integration/kafka/api/ProducerCompressionTest.scala > 83de81cb3f79a6966dd5ef462733d8a22cd6d467 > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala > ee94011894b46864614b97bbd2a98375a7d3f20b > core/src/test/scala/integration/kafka/api/ProducerSendTest.scala > 9ce4bd5ee130ce3cb252b2883a3fd3c9acd742a5 > core/src/test/scala/unit/kafka/KafkaConfigTest.scala > 4764c8976022dfaaebd7ef1cd4ddca55a3ed8a89 > core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala > df5c6ba20f01e497ce896af790cbab40369f1776 > core/src/test/scala/unit/kafka/admin/AdminTest.scala > efb2f8e79b3faef78722774b951fea828cd50374 > core/src/test/scala/unit/kafka/admin/DeleteConsumerGroupTest.scala > 1913ad6d3e6eb29a0c939c4f59f6b688c8c925fa > core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala > fa8ce259a2832ab86f9dda8c1d409b2c42d43ae9 > core/src/test/scala/unit/kafka/admin/TopicCommandTest.scala > c7136f20972614ac47aa57ab13e3c94ef775a4b7 > core/src/test/scala/unit/kafka/api/ApiUtilsTest.scala > 255442526d94157b7a0b5a92e1d6a900aacb7536 > core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala > 5717165f2344823fabe8f7cfafae4bb8af2d949a > core/src/test/scala/unit/kafka/cluster/BrokerEndPointTest.scala > abe511fc1458bfb5a93c152aed81a827cc24ce68 > core/src/test/scala/unit/kafka/common/ConfigTest.scala > 0aca9385bff093b4cb52e42291c9c1d58d9600de > core/src/test/scala/unit/kafka/common/TopicTest.scala > 79532c89c41572ba953c4dc3319a05354927e961 > core/src/test/scala/unit/kafka/consumer/ConsumerIteratorTest.scala > db5302ff02851f9f1a59419b1e071286bff0e142 > core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala > adf08010597b7c6ed72eddf93962497c3209e10f > core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala > 4f124af5c3e946045a78ad1519c37372a72c8985 > > core/src/test/scala/unit/kafka/consumer/ZookeeperConsumerConnectorTest.scala > 359b0f5d14f82d14ee423cde271bb35b7034766d > core/src/test/scala/unit/kafka/coordinator/ConsumerGroupMetadataTest.scala > b69c993a3fa49f7f01dd28e13ce465c2a89eeba5 > core/src/test/scala/unit/kafka/coordinator/CoordinatorMetadataTest.scala > 08854c5e6ec249368206298b2ac2623df18f266a > core/src/test/scala/unit/kafka/coordinator/PartitionAssignorTest.scala > 887cee5a582b5737ba838920399bb9b24bf22382 > core/src/test/scala/unit/kafka/integration/AutoOffsetResetTest.scala > 139dc9a104c024e35fd9bc5ac9333e6bd208b571 > core/src/test/scala/unit/kafka/integration/FetcherTest.scala > facebd8f81c67f26f41a96bce343227bc9b55893 > core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala > 87c631573aa1e0275d6618c4ac3b33e76fa6abd3 > core/src/test/scala/unit/kafka/integration/MinIsrConfigTest.scala > a2c97134d85c637256440b5eb42f594d50f0cfe4 > core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala > 6a758a7db71bdc4794b1e5f264f8ca6d410ff2ba > > core/src/test/scala/unit/kafka/integration/ProducerConsumerTestHarness.scala > 4614a922e739098dbb0ff560d831e26045e32023 > core/src/test/scala/unit/kafka/integration/RollingBounceTest.scala > 12d0733f5edf436315f884bc193da533d9d4a4ee > core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala > 995b05901491bb0dbf0df210d44bd1d7f66fdc82 > core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala > e4bf2df48dd59a251b646b7f96d63ec4b924fc0b > > core/src/test/scala/unit/kafka/javaapi/consumer/ZookeeperConsumerConnectorTest.scala > 74c761dec7afc98667032bdec8359a9aa7c2ecc2 > > core/src/test/scala/unit/kafka/javaapi/message/BaseMessageSetTestCases.scala > 726399e3c7a4157223b5037ff2a03da51236e876 > > core/src/test/scala/unit/kafka/javaapi/message/ByteBufferMessageSetTest.scala > 383fcef02994fde07e669651e522b9e5ee239dd8 > core/src/test/scala/unit/kafka/log/CleanerTest.scala > 8b8249a35322a60ca94cb385a6cad25943dd1cc9 > core/src/test/scala/unit/kafka/log/FileMessageSetTest.scala > cec1caecc51507ae339ebf8f3b8a028b12a1a056 > core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala > 471ddff9bff1bdfa277c071e59e5c6b749b9c74f > core/src/test/scala/unit/kafka/log/LogConfigTest.scala > 3fd5a53f9b0edc0a7a169a185cd3041ea1ae7658 > core/src/test/scala/unit/kafka/log/LogManagerTest.scala > 01dfbc4f8d21f6905327cd4ed6c61d657adc0143 > core/src/test/scala/unit/kafka/log/LogSegmentTest.scala > 03fb3512c4a4450eac83d4cd4b0919baeaa22942 > core/src/test/scala/unit/kafka/log/LogTest.scala > 8e095d652851f05365e1d3bbe3e9e1c3345b7a40 > core/src/test/scala/unit/kafka/log/OffsetIndexTest.scala > 9213a5d9e95d0a5f3009b2cf50baa99765e4cfef > core/src/test/scala/unit/kafka/log/OffsetMapTest.scala > 12ce39e665afd4cb1d8aa8d7d4d7df18b219a141 > core/src/test/scala/unit/kafka/log4j/KafkaLog4jAppenderTest.scala > 41366a14590d318fced0e83d6921d8035fa882da > core/src/test/scala/unit/kafka/message/BaseMessageSetTestCases.scala > dd8847f5f709a0a7b2e7037d5beda9a0dfc054d6 > core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala > 07bc317bcd40cf40fbff1cbd5039672508f293d9 > core/src/test/scala/unit/kafka/message/MessageCompressionTest.scala > 76987d4fa68fd39cf0ce6bc47b05eb32b17bbedb > core/src/test/scala/unit/kafka/message/MessageTest.scala > 11c0f817ca06e4a53bce1009ad6a36aa007cf93a > core/src/test/scala/unit/kafka/message/MessageWriterTest.scala > b08a3432ad1ae49372e4b6cea9b247f6be2889da > core/src/test/scala/unit/kafka/metrics/KafkaTimerTest.scala > 7df74050f99567395d4d01c1600ca77cd917652e > core/src/test/scala/unit/kafka/metrics/MetricsTest.scala > b42101b85fa4aa6a2249e7b04ec0f61e51c81c3f > core/src/test/scala/unit/kafka/network/SocketServerTest.scala > 7dc2fad542ea553ee888543dd215eb41ea57d509 > core/src/test/scala/unit/kafka/producer/AsyncProducerTest.scala > be4bb878dc49f05caf55b36e9218ed19b1c56253 > core/src/test/scala/unit/kafka/producer/ProducerTest.scala > 4d2536b462c030f4f269bad024847839e63337c2 > core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala > 8c3fb7a393db019a29b284b26670f63c25d6a4c6 > core/src/test/scala/unit/kafka/server/AdvertiseBrokerTest.scala > e899b023b3153542f481b82e92b258e4595b23dc > core/src/test/scala/unit/kafka/server/DelayedOperationTest.scala > f3ab3f4ff8eb1aa6b2ab87ba75f72eceb6649620 > core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala > 7877f6ca1845c2edbf96d4a9783a07a552db8f07 > core/src/test/scala/unit/kafka/server/HighwatermarkPersistenceTest.scala > 60cd8249e6ec03349e20bb0a7226ea9cd66e6b17 > core/src/test/scala/unit/kafka/server/ISRExpirationTest.scala > 90529faf11dca65c3ef6bcb27ad72557bc36f939 > core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala > c487f361949b2ca2b6d1b5e2c7fb9ba83c8e53c1 > core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala > 2428dbd7197a58cf4cad42ef82b385dab3a2b15e > core/src/test/scala/unit/kafka/server/LeaderElectionTest.scala > f1977d850a5bf1125260949101fa3485b29bd4e6 > core/src/test/scala/unit/kafka/server/LogOffsetTest.scala > e57c1dec2dee4b9a64ffe5906d12676fd877319b > core/src/test/scala/unit/kafka/server/LogRecoveryTest.scala > 7688f26fe42c4481fe2da7a1d50459805be0ebc1 > core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala > 528525b719ec916e16f8b3ae3715bec4b5dcc47d > core/src/test/scala/unit/kafka/server/ReplicaFetchTest.scala > a3a03db88c4c3b2949c4ff7f26f2f00498884b18 > core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala > 00d59337a99ac135e8689bd1ecd928f7b1423d79 > core/src/test/scala/unit/kafka/server/ServerGenerateBrokerIdTest.scala > 12269cde06aab953dcd898821a8afaa5c4290e77 > core/src/test/scala/unit/kafka/server/ServerShutdownTest.scala > 95534e36c29023565fedf6bf0fe537ef270c2420 > core/src/test/scala/unit/kafka/server/ServerStartupTest.scala > 60e10b3d5adda152a01425d98d45ca373a63bebd > core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala > 09a0961f73969aca3b054ed3ff6b181a6500364a > core/src/test/scala/unit/kafka/utils/ByteBoundedBlockingQueueTest.scala > fd8cf7bf04c5f8e6166f1e3dd5ab369b478c02ee > core/src/test/scala/unit/kafka/utils/CommandLineUtilsTest.scala > 6380b6e623e7a6fcaf7b44473291075a0aa010a8 > core/src/test/scala/unit/kafka/utils/IteratorTemplateTest.scala > fbd245cad0afab146354fdcc76193d784e1997d9 > core/src/test/scala/unit/kafka/utils/JsonTest.scala > 93550e8f24071f88eb1ea5b41373efee27e4b8b7 > core/src/test/scala/unit/kafka/utils/ReplicationUtilsTest.scala > c96c0ffd958d63c09880d436b2e5ae96f51ead36 > core/src/test/scala/unit/kafka/utils/SchedulerTest.scala > cfea63b88e2590cbd7a1e7cb2c8cacf054bd5568 > core/src/test/scala/unit/kafka/utils/TestUtils.scala > 17e9fe4c159a29033fe9a287db6ced2fdc3fa9d1 > core/src/test/scala/unit/kafka/utils/timer/TimerTaskListTest.scala > 052aecdc32ae14afe6aabf942ed7c0028ed9f979 > core/src/test/scala/unit/kafka/utils/timer/TimerTest.scala > 8507592d1f374248cc7f56ef786544562ca8ad9c > core/src/test/scala/unit/kafka/zk/ZKEphemeralTest.scala > 2be161985657293e83a3fd4d3d15faa30e9a119e > core/src/test/scala/unit/kafka/zk/ZKPathTest.scala > d3e44c62e272b01edd51553270e7d667e55f6e44 > core/src/test/scala/unit/kafka/zk/ZooKeeperTestHarness.scala > 1f4d10d25ab3b907689ead7701d2c868ba703952 > > Diff: https://reviews.apache.org/r/35615/diff/ > > > Testing > ------- > > > Thanks, > > Alexander Pakulov > >
