You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/10/28 15:06:53 UTC
[GitHub] [kafka] chia7712 opened a new pull request #9520: MINOR: replace test "expected" parameter by assertThrows
chia7712 opened a new pull request #9520:
URL: https://github.com/apache/kafka/pull/9520
```assertThrows``` can make checks in testing more flexible and accurate so I think it is worth substituting ```assertThrows``` for "expected" parameter.
I don't replace all "expected" parameter. For example, the one-line simple test is fine to use "expected" parameter.
### 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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-757429406
fix conflicting files
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r553439047
##########
File path: core/src/test/scala/unit/kafka/utils/QuotaUtilsTest.scala
##########
@@ -17,24 +17,21 @@
package kafka.utils
-import java.util.concurrent.TimeUnit
-
import org.apache.kafka.common.MetricName
-import org.apache.kafka.common.metrics.{KafkaMetric, MetricConfig, Quota, QuotaViolationException}
import org.apache.kafka.common.metrics.stats.{Rate, Value}
-
-import scala.jdk.CollectionConverters._
+import org.apache.kafka.common.metrics.{KafkaMetric, MetricConfig, Quota, QuotaViolationException}
import org.junit.Assert._
import org.junit.Test
-import org.scalatest.Assertions.assertThrows
+
+import java.util.concurrent.TimeUnit
class QuotaUtilsTest {
private val time = new MockTime
private val numSamples = 10
private val sampleWindowSec = 1
private val maxThrottleTimeMs = 500
- private val metricName = new MetricName("test-metric", "groupA", "testA", Map.empty.asJava)
+ private val metricName = new MetricName("test-metric", "groupA", "testA", java.util.Collections.emptyMap())
Review comment:
Please give me one second to revert those changes :(
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-756800352
Looking at the test results, it looks like there are some real failures. For example:
`Build / JDK 8 / kafka.utils.QuotaUtilsTest.testBoundedThrottleTimeThrowsExceptionIfProvidedNonRateMetric`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r549567477
##########
File path: clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java
##########
@@ -444,34 +444,30 @@ public void testAddPartitionToTransactionRetainsRetryBackoffWhenPartitionsAlread
assertEquals(DEFAULT_RETRY_BACKOFF_MS, handler.retryBackoffMs());
}
- @Test(expected = IllegalStateException.class)
+ @Test
public void testMaybeAddPartitionToTransactionBeforeInitTransactions() {
- transactionManager.failIfNotReadyForSend();
- transactionManager.maybeAddPartitionToTransaction(new TopicPartition("foo", 0));
+ assertThrows(IllegalStateException.class, () -> transactionManager.failIfNotReadyForSend());
}
- @Test(expected = IllegalStateException.class)
+ @Test
public void testMaybeAddPartitionToTransactionBeforeBeginTransaction() {
doInitTransactions();
- transactionManager.failIfNotReadyForSend();
- transactionManager.maybeAddPartitionToTransaction(new TopicPartition("foo", 0));
+ assertThrows(IllegalStateException.class, () -> transactionManager.failIfNotReadyForSend());
}
- @Test(expected = KafkaException.class)
+ @Test
public void testMaybeAddPartitionToTransactionAfterAbortableError() {
doInitTransactions();
transactionManager.beginTransaction();
transactionManager.transitionToAbortableError(new KafkaException());
- transactionManager.failIfNotReadyForSend();
- transactionManager.maybeAddPartitionToTransaction(new TopicPartition("foo", 0));
+ assertThrows(KafkaException.class, () -> transactionManager.failIfNotReadyForSend());
}
- @Test(expected = KafkaException.class)
+ @Test
public void testMaybeAddPartitionToTransactionAfterFatalError() {
doInitTransactions();
transactionManager.transitionToFatalError(new KafkaException());
- transactionManager.failIfNotReadyForSend();
- transactionManager.maybeAddPartitionToTransaction(new TopicPartition("foo", 0));
+ assertThrows(KafkaException.class, () -> transactionManager.failIfNotReadyForSend());
Review comment:
https://github.com/apache/kafka/commit/717c55be971df862c55f55d245b9997f1d6f998c moved the check of state out of ```maybeAddPartitionToTransaction``` and it added ```failIfNotReadyForSend``` to all test cases to make them throw exception. I will update the test cases name to avoid confusion.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 edited a comment on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 edited a comment on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-750005283
@ijuma the latest commit replaces all ExpectedException or @Test(expected = Exception.class) by assertThrows. It seems to me we can get closer to junit 5 if this PR is got into trunk. Furthermore, I'd like to file following PR to remove all junit 4 code.
1. Replace ScalaTest by junit
2. Replace junit 4 APIs by Junit 5 for each module
1. rewrite parameter tests
2. replace ```org.junit``` by ```org.junit.jupiter.api```
3. replace ```IntegrationTest``` by tag ```Integration```
4. replace ```useJUnitPlatform``` by ```useJUnit```
5. remove ```testCompile libs.junitVintageEngine```
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-757426244
I would do the minimal set of changes to make the test suite pass with JUnit 5.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-756800352
Looking at the test results, it looks like there are some real failures. For example:
`Build / JDK 8 / kafka.utils.QuotaUtilsTest.testBoundedThrottleTimeThrowsExceptionIfProvidedNonRateMetric`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 merged pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 merged pull request #9520:
URL: https://github.com/apache/kafka/pull/9520
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-756282441
I have reverted most unrelated changes. see https://github.com/apache/kafka/pull/9520/commits/d0e2236e96db0b514f4d2c07641eb236feb1de74
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-757421003
> Will you do the core conversion to JUnit 5 next?
of course. Should we keep using ```ScalaTest``` and ```hamcrest```? They are not in conflict with Junit 5 but it results in various code style in testing.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r552705598
##########
File path: clients/src/test/java/org/apache/kafka/common/record/MemoryRecordsBuilderTest.java
##########
@@ -504,20 +510,21 @@ public void writePastLimit() {
}
}
- @Test(expected = IllegalArgumentException.class)
+ @Test
public void testAppendAtInvalidOffset() {
ByteBuffer buffer = ByteBuffer.allocate(1024);
buffer.position(bufferOffset);
long logAppendTime = System.currentTimeMillis();
- MemoryRecordsBuilder builder = new MemoryRecordsBuilder(buffer, RecordBatch.MAGIC_VALUE_V1, compressionType,
+ MemoryRecordsBuilder builder = new MemoryRecordsBuilder(buffer, RecordBatch.MAGIC_VALUE_V2, compressionType,
Review comment:
Unrelated to the changes in this PR then, just something you noticed? Worth a line in the PR description to avoid confusion.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-748432641
@ijuma Any thoughts to this PR?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-756299473
> Can you please remove the last commit?
done!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 edited a comment on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 edited a comment on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-750005283
@ijuma the latest commit replaces all ExpectedException or @Test(expected = Exception.class) by assertThrows. It seems to me we can get closer to junit 5 if this PR is got into trunk. Furthermore, I'd like to file following PR to remove all junit 4 code.
1. Replace ```org.hamcrest``` by junit
1. Replace ScalaTest by junit
2. Replace junit 4 APIs by Junit 5 for each module
1. rewrite parameter tests
2. replace ```org.junit``` by ```org.junit.jupiter.api```
3. replace ```IntegrationTest``` by tag ```Integration```
4. replace ```useJUnitPlatform``` by ```useJUnit```
5. remove ```testCompile libs.junitVintageEngine```
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r546320434
##########
File path: clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java
##########
@@ -196,12 +197,12 @@ public void testHierarchicalSensors() {
assertNull(metrics.childrenSensors().get(grandchild));
}
- @Test(expected = IllegalArgumentException.class)
+ @Test
public void testBadSensorHierarchy() {
Sensor p = metrics.sensor("parent");
Sensor c1 = metrics.sensor("child1", p);
Sensor c2 = metrics.sensor("child2", p);
- metrics.sensor("gc", c1, c2); // should fail
+ assertThrows(IllegalArgumentException.class, () -> metrics.sensor("gc", c1, c2)); // should fail
Review comment:
We can remove the comment, it's redundant.
##########
File path: clients/src/test/java/org/apache/kafka/common/record/ByteBufferLogInputStreamTest.java
##########
@@ -116,8 +117,7 @@ public void iteratorRaisesOnTooLargeRecords() {
buffer.flip();
ByteBufferLogInputStream logInputStream = new ByteBufferLogInputStream(buffer, 25);
- assertNotNull(logInputStream.nextBatch());
- logInputStream.nextBatch();
+ assertThrows(CorruptRecordException.class, logInputStream::nextBatch);
Review comment:
Hmm, I think this was intended to fail on the second `nextBatch`, so maybe we should adjust the test.
##########
File path: core/src/test/scala/integration/kafka/api/EndToEndAuthorizationTest.scala
##########
@@ -411,14 +411,13 @@ abstract class EndToEndAuthorizationTest extends IntegrationTestHarness with Sas
* Tests that a consumer fails to consume messages without the appropriate
* ACL set.
*/
- @Test(expected = classOf[KafkaException])
+ @Test
def testNoConsumeWithoutDescribeAclViaAssign(): Unit = {
noConsumeWithoutDescribeAclSetup()
val consumer = createConsumer()
consumer.assign(List(tp).asJava)
// the exception is expected when the consumer attempts to lookup offsets
- consumeRecords(consumer)
- confirmReauthenticationMetrics()
Review comment:
We should not remove this, right?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r549419585
##########
File path: clients/src/test/java/org/apache/kafka/common/memory/GarbageCollectedMemoryPoolTest.java
##########
@@ -23,68 +23,74 @@
import org.junit.Assert;
import org.junit.Test;
+import static org.junit.Assert.assertThrows;
+
public class GarbageCollectedMemoryPoolTest {
- @Test(expected = IllegalArgumentException.class)
- public void testZeroSize() throws Exception {
- new GarbageCollectedMemoryPool(0, 7, true, null);
+ @Test
+ public void testZeroSize() {
+ assertThrows(IllegalArgumentException.class,
+ () -> new GarbageCollectedMemoryPool(0, 7, true, null));
}
- @Test(expected = IllegalArgumentException.class)
- public void testNegativeSize() throws Exception {
- new GarbageCollectedMemoryPool(-1, 7, false, null);
+ @Test
+ public void testNegativeSize() {
+ assertThrows(IllegalArgumentException.class,
+ () -> new GarbageCollectedMemoryPool(-1, 7, false, null));
}
- @Test(expected = IllegalArgumentException.class)
- public void testZeroMaxAllocation() throws Exception {
- new GarbageCollectedMemoryPool(100, 0, true, null);
+ @Test
+ public void testZeroMaxAllocation() {
+ assertThrows(IllegalArgumentException.class,
+ () -> new GarbageCollectedMemoryPool(100, 0, true, null));
}
- @Test(expected = IllegalArgumentException.class)
- public void testNegativeMaxAllocation() throws Exception {
- new GarbageCollectedMemoryPool(100, -1, false, null);
+ @Test
+ public void testNegativeMaxAllocation() {
+ assertThrows(IllegalArgumentException.class,
+ () -> new GarbageCollectedMemoryPool(100, -1, false, null));
}
- @Test(expected = IllegalArgumentException.class)
- public void testMaxAllocationLargerThanSize() throws Exception {
- new GarbageCollectedMemoryPool(100, 101, true, null);
+ @Test
+ public void testMaxAllocationLargerThanSize() {
+ assertThrows(IllegalArgumentException.class,
+ () -> new GarbageCollectedMemoryPool(100, 101, true, null));
}
- @Test(expected = IllegalArgumentException.class)
- public void testAllocationOverMaxAllocation() throws Exception {
+ @Test
+ public void testAllocationOverMaxAllocation() {
GarbageCollectedMemoryPool pool = new GarbageCollectedMemoryPool(1000, 10, false, null);
- pool.tryAllocate(11);
+ assertThrows(IllegalArgumentException.class, () -> pool.tryAllocate(11));
}
- @Test(expected = IllegalArgumentException.class)
- public void testAllocationZero() throws Exception {
+ @Test
+ public void testAllocationZero() {
GarbageCollectedMemoryPool pool = new GarbageCollectedMemoryPool(1000, 10, true, null);
- pool.tryAllocate(0);
+ assertThrows(IllegalArgumentException.class, () -> pool.tryAllocate(0));
}
- @Test(expected = IllegalArgumentException.class)
- public void testAllocationNegative() throws Exception {
+ @Test
+ public void testAllocationNegative() {
GarbageCollectedMemoryPool pool = new GarbageCollectedMemoryPool(1000, 10, false, null);
- pool.tryAllocate(-1);
+ assertThrows(IllegalArgumentException.class, () -> pool.tryAllocate(-1));
}
- @Test(expected = IllegalArgumentException.class)
- public void testReleaseNull() throws Exception {
+ @Test
+ public void testReleaseNull() {
GarbageCollectedMemoryPool pool = new GarbageCollectedMemoryPool(1000, 10, true, null);
- pool.release(null);
+ assertThrows(IllegalArgumentException.class, () -> pool.release(null));
}
- @Test(expected = IllegalArgumentException.class)
- public void testReleaseForeignBuffer() throws Exception {
+ @Test
+ public void testReleaseForeignBuffer() {
GarbageCollectedMemoryPool pool = new GarbageCollectedMemoryPool(1000, 10, true, null);
ByteBuffer fellOffATruck = ByteBuffer.allocate(1);
- pool.release(fellOffATruck);
- pool.close();
Review comment:
Do we still need to call `close` to avoid a leak?
##########
File path: clients/src/test/java/org/apache/kafka/common/record/MemoryRecordsBuilderTest.java
##########
@@ -504,20 +510,21 @@ public void writePastLimit() {
}
}
- @Test(expected = IllegalArgumentException.class)
+ @Test
public void testAppendAtInvalidOffset() {
ByteBuffer buffer = ByteBuffer.allocate(1024);
buffer.position(bufferOffset);
long logAppendTime = System.currentTimeMillis();
- MemoryRecordsBuilder builder = new MemoryRecordsBuilder(buffer, RecordBatch.MAGIC_VALUE_V1, compressionType,
+ MemoryRecordsBuilder builder = new MemoryRecordsBuilder(buffer, RecordBatch.MAGIC_VALUE_V2, compressionType,
Review comment:
Why did we change the magic value version here?
##########
File path: clients/src/test/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticatorTest.java
##########
@@ -67,8 +68,7 @@ public void testOversizeRequest() throws IOException {
invocation.<ByteBuffer>getArgument(0).putInt(SaslServerAuthenticator.MAX_RECEIVE_SIZE + 1);
return 4;
});
- authenticator.authenticate();
- verify(transportLayer).read(any(ByteBuffer.class));
Review comment:
Why don't we need the `verify` call?
##########
File path: connect/api/src/test/java/org/apache/kafka/connect/data/SchemaBuilderTest.java
##########
@@ -293,16 +295,13 @@ public void testEmptyStruct() {
new Struct(emptyStructSchema);
}
- @Test(expected = SchemaBuilderException.class)
+ @Test
public void testDuplicateFields() {
- final Schema schema = SchemaBuilder.struct()
- .name("testing")
- .field("id", SchemaBuilder.string().doc("").build())
- .field("id", SchemaBuilder.string().doc("").build())
- .build();
- final Struct struct = new Struct(schema)
- .put("id", "testing");
- struct.validate();
+ assertThrows(SchemaBuilderException.class, () -> SchemaBuilder.struct()
+ .name("testing")
+ .field("id", SchemaBuilder.string().doc("").build())
+ .field("id", SchemaBuilder.string().doc("").build())
+ .build());
Review comment:
It seems that we are not testing the `validate` method anymore.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-748854295
> One more thing: we seem to sometimes use the scalatest asserts and sometimes the JUnit ones for the Scala code. I would suggest using the JUnit ones consistently as it will make it easier to convert the tests to JUnit 5.
Except for ```org.scalatest.Assertions.withClue```, others package from ```org.scalatest.Assertions``` are replaced by Junit.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-757469199
@ijuma Thanks for all your great reviews!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-755925570
rebase code to fix conflicts
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r549417155
##########
File path: clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java
##########
@@ -192,11 +192,12 @@ public void testConstructorWithSerializers() {
new KafkaProducer<>(producerProps, new ByteArraySerializer(), new ByteArraySerializer()).close();
}
- @Test(expected = ConfigException.class)
+ @Test
public void testNoSerializerProvided() {
Properties producerProps = new Properties();
producerProps.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9000");
- new KafkaProducer(producerProps);
+
Review comment:
Was this new line intentional?
##########
File path: clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java
##########
@@ -444,34 +444,30 @@ public void testAddPartitionToTransactionRetainsRetryBackoffWhenPartitionsAlread
assertEquals(DEFAULT_RETRY_BACKOFF_MS, handler.retryBackoffMs());
}
- @Test(expected = IllegalStateException.class)
+ @Test
public void testMaybeAddPartitionToTransactionBeforeInitTransactions() {
- transactionManager.failIfNotReadyForSend();
- transactionManager.maybeAddPartitionToTransaction(new TopicPartition("foo", 0));
+ assertThrows(IllegalStateException.class, () -> transactionManager.failIfNotReadyForSend());
}
- @Test(expected = IllegalStateException.class)
+ @Test
public void testMaybeAddPartitionToTransactionBeforeBeginTransaction() {
doInitTransactions();
- transactionManager.failIfNotReadyForSend();
- transactionManager.maybeAddPartitionToTransaction(new TopicPartition("foo", 0));
+ assertThrows(IllegalStateException.class, () -> transactionManager.failIfNotReadyForSend());
Review comment:
Same as above.
##########
File path: clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java
##########
@@ -444,34 +444,30 @@ public void testAddPartitionToTransactionRetainsRetryBackoffWhenPartitionsAlread
assertEquals(DEFAULT_RETRY_BACKOFF_MS, handler.retryBackoffMs());
}
- @Test(expected = IllegalStateException.class)
+ @Test
public void testMaybeAddPartitionToTransactionBeforeInitTransactions() {
- transactionManager.failIfNotReadyForSend();
- transactionManager.maybeAddPartitionToTransaction(new TopicPartition("foo", 0));
+ assertThrows(IllegalStateException.class, () -> transactionManager.failIfNotReadyForSend());
}
- @Test(expected = IllegalStateException.class)
+ @Test
public void testMaybeAddPartitionToTransactionBeforeBeginTransaction() {
doInitTransactions();
- transactionManager.failIfNotReadyForSend();
- transactionManager.maybeAddPartitionToTransaction(new TopicPartition("foo", 0));
+ assertThrows(IllegalStateException.class, () -> transactionManager.failIfNotReadyForSend());
}
- @Test(expected = KafkaException.class)
+ @Test
public void testMaybeAddPartitionToTransactionAfterAbortableError() {
doInitTransactions();
transactionManager.beginTransaction();
transactionManager.transitionToAbortableError(new KafkaException());
- transactionManager.failIfNotReadyForSend();
- transactionManager.maybeAddPartitionToTransaction(new TopicPartition("foo", 0));
+ assertThrows(KafkaException.class, () -> transactionManager.failIfNotReadyForSend());
Review comment:
Same as above.
##########
File path: clients/src/test/java/org/apache/kafka/clients/producer/MockProducerTest.java
##########
@@ -663,12 +623,6 @@ public void shouldPreserveOffsetsFromCommitByGroupMetadataOnAbortIfTransactionsA
producer.beginTransaction();
String group2 = "g2";
- Map<TopicPartition, OffsetAndMetadata> groupCommit2 = new HashMap<TopicPartition, OffsetAndMetadata>() {
- {
- put(new TopicPartition(topic, 2), new OffsetAndMetadata(53L, null));
- put(new TopicPartition(topic, 3), new OffsetAndMetadata(84L, null));
- }
- };
producer.sendOffsetsToTransaction(groupCommit, new ConsumerGroupMetadata(group2));
Review comment:
I think this was intended to be `groupCommit2`, right?
##########
File path: clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java
##########
@@ -444,34 +444,30 @@ public void testAddPartitionToTransactionRetainsRetryBackoffWhenPartitionsAlread
assertEquals(DEFAULT_RETRY_BACKOFF_MS, handler.retryBackoffMs());
}
- @Test(expected = IllegalStateException.class)
+ @Test
public void testMaybeAddPartitionToTransactionBeforeInitTransactions() {
- transactionManager.failIfNotReadyForSend();
- transactionManager.maybeAddPartitionToTransaction(new TopicPartition("foo", 0));
+ assertThrows(IllegalStateException.class, () -> transactionManager.failIfNotReadyForSend());
}
- @Test(expected = IllegalStateException.class)
+ @Test
public void testMaybeAddPartitionToTransactionBeforeBeginTransaction() {
doInitTransactions();
- transactionManager.failIfNotReadyForSend();
- transactionManager.maybeAddPartitionToTransaction(new TopicPartition("foo", 0));
+ assertThrows(IllegalStateException.class, () -> transactionManager.failIfNotReadyForSend());
}
- @Test(expected = KafkaException.class)
+ @Test
public void testMaybeAddPartitionToTransactionAfterAbortableError() {
doInitTransactions();
transactionManager.beginTransaction();
transactionManager.transitionToAbortableError(new KafkaException());
- transactionManager.failIfNotReadyForSend();
- transactionManager.maybeAddPartitionToTransaction(new TopicPartition("foo", 0));
+ assertThrows(KafkaException.class, () -> transactionManager.failIfNotReadyForSend());
}
- @Test(expected = KafkaException.class)
+ @Test
public void testMaybeAddPartitionToTransactionAfterFatalError() {
doInitTransactions();
transactionManager.transitionToFatalError(new KafkaException());
- transactionManager.failIfNotReadyForSend();
- transactionManager.maybeAddPartitionToTransaction(new TopicPartition("foo", 0));
+ assertThrows(KafkaException.class, () -> transactionManager.failIfNotReadyForSend());
Review comment:
Same as above.
##########
File path: clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java
##########
@@ -444,34 +444,30 @@ public void testAddPartitionToTransactionRetainsRetryBackoffWhenPartitionsAlread
assertEquals(DEFAULT_RETRY_BACKOFF_MS, handler.retryBackoffMs());
}
- @Test(expected = IllegalStateException.class)
+ @Test
public void testMaybeAddPartitionToTransactionBeforeInitTransactions() {
- transactionManager.failIfNotReadyForSend();
- transactionManager.maybeAddPartitionToTransaction(new TopicPartition("foo", 0));
+ assertThrows(IllegalStateException.class, () -> transactionManager.failIfNotReadyForSend());
Review comment:
This change doesn't seem right. The test is intended to test `maybeAddPartitionToTransaction`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r549569836
##########
File path: connect/api/src/test/java/org/apache/kafka/connect/data/SchemaBuilderTest.java
##########
@@ -293,16 +295,13 @@ public void testEmptyStruct() {
new Struct(emptyStructSchema);
}
- @Test(expected = SchemaBuilderException.class)
+ @Test
public void testDuplicateFields() {
- final Schema schema = SchemaBuilder.struct()
- .name("testing")
- .field("id", SchemaBuilder.string().doc("").build())
- .field("id", SchemaBuilder.string().doc("").build())
- .build();
- final Struct struct = new Struct(schema)
- .put("id", "testing");
- struct.validate();
+ assertThrows(SchemaBuilderException.class, () -> SchemaBuilder.struct()
+ .name("testing")
+ .field("id", SchemaBuilder.string().doc("").build())
+ .field("id", SchemaBuilder.string().doc("").build())
+ .build());
Review comment:
```StructTest``` has test cases for ```validate``` method.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-750351742
> I was about halfway through the review of the second to last commit. :) Let's get this PR over the line and not change it further please.
I am so sorry my force push obstructed you from reviewing. I will never do that again :(
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-750005283
@ijuma the latest commit replaces all ExpectedException or @Test(expected = Exception.class) by assertThrows. It seems to me we can get closer to junit 5 if this PR is got into trunk. Furthermore, I'd like to file following PR to remove all junit 4 code.
1. Replace ScalaTest by junit
2. Replace junit 4 APIs by Junit 5 for each module
1. rewrite parameter tests
2. replace ```org.junit``` by ```org.junit.jupiter.api```
3. replace ```IntegrationTest``` by tag ```Integration```
4. replace ```useJUnitPlatform``` by ```useJUnit```
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r553392601
##########
File path: core/src/test/scala/unit/kafka/utils/QuotaUtilsTest.scala
##########
@@ -17,24 +17,21 @@
package kafka.utils
-import java.util.concurrent.TimeUnit
-
import org.apache.kafka.common.MetricName
-import org.apache.kafka.common.metrics.{KafkaMetric, MetricConfig, Quota, QuotaViolationException}
import org.apache.kafka.common.metrics.stats.{Rate, Value}
-
-import scala.jdk.CollectionConverters._
+import org.apache.kafka.common.metrics.{KafkaMetric, MetricConfig, Quota, QuotaViolationException}
import org.junit.Assert._
import org.junit.Test
-import org.scalatest.Assertions.assertThrows
+
+import java.util.concurrent.TimeUnit
class QuotaUtilsTest {
private val time = new MockTime
private val numSamples = 10
private val sampleWindowSec = 1
private val maxThrottleTimeMs = 500
- private val metricName = new MetricName("test-metric", "groupA", "testA", Map.empty.asJava)
+ private val metricName = new MetricName("test-metric", "groupA", "testA", java.util.Collections.emptyMap())
Review comment:
my bad and will revert it. (IntelliJ complained about the erased types)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r539422760
##########
File path: clients/src/test/java/org/apache/kafka/clients/NetworkClientTest.java
##########
@@ -117,13 +118,12 @@ public void setup() {
selector.reset();
}
- @Test(expected = IllegalStateException.class)
+ @Test
public void testSendToUnreadyNode() {
MetadataRequest.Builder builder = new MetadataRequest.Builder(Collections.singletonList("test"), true);
long now = time.milliseconds();
ClientRequest request = client.newClientRequest("5", builder, now, false);
- client.send(request, now);
- client.poll(1, time.milliseconds());
Review comment:
```client.send(request, now);``` throws exception so ```client.poll(1, time.milliseconds());``` was never reached.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r553437304
##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorNodeTest.java
##########
@@ -49,18 +49,17 @@
public class ProcessorNodeTest {
@SuppressWarnings("unchecked")
- @Test(expected = StreamsException.class)
+ @Test
public void shouldThrowStreamsExceptionIfExceptionCaughtDuringInit() {
final ProcessorNode node = new ProcessorNode("name", new ExceptionalProcessor(), Collections.emptySet());
- node.init(null);
+ assertThrows(StreamsException.class, () -> node.init(null));
}
@SuppressWarnings("unchecked")
- @Test(expected = StreamsException.class)
+ @Test
public void shouldThrowStreamsExceptionIfExceptionCaughtDuringClose() {
final ProcessorNode node = new ProcessorNode("name", new ExceptionalProcessor(), Collections.emptySet());
- node.init(null);
- node.close();
Review comment:
I don't think this is necessarily the case. The failure during init can happen after some resources have been allocated, right?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r553388205
##########
File path: core/src/test/scala/unit/kafka/server/DynamicBrokerConfigTest.scala
##########
@@ -343,7 +340,7 @@ class DynamicBrokerConfigTest {
class TestAuthorizer extends Authorizer with Reconfigurable {
@volatile var superUsers = ""
- override def start(serverInfo: AuthorizerServerInfo): util.Map[Endpoint, _ <: CompletionStage[Void]] = Map.empty.asJava
+ override def start(serverInfo: AuthorizerServerInfo): util.Map[Endpoint, _ <: CompletionStage[Void]] = util.Collections.emptyMap()
Review comment:
Why did we change this?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r552713559
##########
File path: clients/src/test/java/org/apache/kafka/common/record/MemoryRecordsBuilderTest.java
##########
@@ -504,20 +510,21 @@ public void writePastLimit() {
}
}
- @Test(expected = IllegalArgumentException.class)
+ @Test
public void testAppendAtInvalidOffset() {
ByteBuffer buffer = ByteBuffer.allocate(1024);
buffer.position(bufferOffset);
long logAppendTime = System.currentTimeMillis();
- MemoryRecordsBuilder builder = new MemoryRecordsBuilder(buffer, RecordBatch.MAGIC_VALUE_V1, compressionType,
+ MemoryRecordsBuilder builder = new MemoryRecordsBuilder(buffer, RecordBatch.MAGIC_VALUE_V2, compressionType,
Review comment:
will copy that
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r553394406
##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorNodeTest.java
##########
@@ -49,18 +49,17 @@
public class ProcessorNodeTest {
@SuppressWarnings("unchecked")
- @Test(expected = StreamsException.class)
+ @Test
public void shouldThrowStreamsExceptionIfExceptionCaughtDuringInit() {
final ProcessorNode node = new ProcessorNode("name", new ExceptionalProcessor(), Collections.emptySet());
- node.init(null);
+ assertThrows(StreamsException.class, () -> node.init(null));
}
@SuppressWarnings("unchecked")
- @Test(expected = StreamsException.class)
+ @Test
public void shouldThrowStreamsExceptionIfExceptionCaughtDuringClose() {
final ProcessorNode node = new ProcessorNode("name", new ExceptionalProcessor(), Collections.emptySet());
- node.init(null);
- node.close();
Review comment:
It fails to init so the close is unnecessary.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r553369873
##########
File path: core/src/test/scala/unit/kafka/zookeeper/ZooKeeperClientTest.scala
##########
@@ -715,7 +711,7 @@ class ZooKeeperClientTest extends ZooKeeperTestHarness {
private def cleanMetricsRegistry(): Unit = {
val metrics = KafkaYammerMetrics.defaultRegistry
- metrics.allMetrics.keySet.forEach(metrics.removeMetric)
+ metrics.allMetrics.keySet.forEach(m => metrics.removeMetric(m))
Review comment:
Why did we change this?
##########
File path: core/src/test/scala/integration/kafka/api/TransactionsTest.scala
##########
@@ -407,46 +406,40 @@ class TransactionsTest extends KafkaServerTestHarness {
TestUtils.waitUntilTrue(() => offsetAndMetadata.equals(consumer.committed(Set(tp).asJava).get(tp)), "cannot read committed offset")
}
- @Test(expected = classOf[TimeoutException])
+ @Test
def testInitTransactionsTimeout(): Unit = {
testTimeout(false, producer => producer.initTransactions())
}
- @Test(expected = classOf[TimeoutException])
+ @Test
def testSendOffsetsToTransactionTimeout(): Unit = {
testTimeout(true, producer => producer.sendOffsetsToTransaction(
Map(new TopicPartition(topic1, 0) -> new OffsetAndMetadata(0)).asJava, "test-group"))
}
- @Test(expected = classOf[TimeoutException])
+ @Test
def testCommitTransactionTimeout(): Unit = {
testTimeout(true, producer => producer.commitTransaction())
}
- @Test(expected = classOf[TimeoutException])
+ @Test
def testAbortTransactionTimeout(): Unit = {
testTimeout(true, producer => producer.abortTransaction())
}
- def testTimeout(needInitAndSendMsg: Boolean,
+ private def testTimeout(needInitAndSendMsg: Boolean,
timeoutProcess: KafkaProducer[Array[Byte], Array[Byte]] => Unit): Unit = {
- val producer = createTransactionalProducer("transactionProducer", maxBlockMs = 1000)
-
+ val producer = createTransactionalProducer("transactionProducer", maxBlockMs = 3000)
Review comment:
Why did we increase maxBlockMs here?
##########
File path: core/src/test/scala/unit/kafka/utils/QuotaUtilsTest.scala
##########
@@ -17,24 +17,21 @@
package kafka.utils
-import java.util.concurrent.TimeUnit
-
import org.apache.kafka.common.MetricName
-import org.apache.kafka.common.metrics.{KafkaMetric, MetricConfig, Quota, QuotaViolationException}
import org.apache.kafka.common.metrics.stats.{Rate, Value}
-
-import scala.jdk.CollectionConverters._
+import org.apache.kafka.common.metrics.{KafkaMetric, MetricConfig, Quota, QuotaViolationException}
import org.junit.Assert._
import org.junit.Test
-import org.scalatest.Assertions.assertThrows
+
+import java.util.concurrent.TimeUnit
class QuotaUtilsTest {
private val time = new MockTime
private val numSamples = 10
private val sampleWindowSec = 1
private val maxThrottleTimeMs = 500
- private val metricName = new MetricName("test-metric", "groupA", "testA", Map.empty.asJava)
+ private val metricName = new MetricName("test-metric", "groupA", "testA", java.util.Collections.emptyMap())
Review comment:
Why did you change this?
##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorNodeTest.java
##########
@@ -49,18 +49,17 @@
public class ProcessorNodeTest {
@SuppressWarnings("unchecked")
- @Test(expected = StreamsException.class)
+ @Test
public void shouldThrowStreamsExceptionIfExceptionCaughtDuringInit() {
final ProcessorNode node = new ProcessorNode("name", new ExceptionalProcessor(), Collections.emptySet());
- node.init(null);
+ assertThrows(StreamsException.class, () -> node.init(null));
}
@SuppressWarnings("unchecked")
- @Test(expected = StreamsException.class)
+ @Test
public void shouldThrowStreamsExceptionIfExceptionCaughtDuringClose() {
final ProcessorNode node = new ProcessorNode("name", new ExceptionalProcessor(), Collections.emptySet());
- node.init(null);
- node.close();
Review comment:
Should we keep the `close`?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-750345209
@chia7712 Thanks. I was about halfway through the review of the second to last commit. :) Let's get this PR over the line and not change it further please. Yes, I had filed a few JIRAs for migrating to JUnit 5, see https://issues.apache.org/jira/browse/KAFKA-7339.
I think we should do it on a module per module basis to make it easier to review (the JIRAs are structured in that way). I was going to try the clients module after this PR is merged. Feel free to take one of the other ones. `core` would be a good candidate perhaps.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r553390132
##########
File path: core/src/test/scala/integration/kafka/api/TransactionsTest.scala
##########
@@ -407,46 +406,40 @@ class TransactionsTest extends KafkaServerTestHarness {
TestUtils.waitUntilTrue(() => offsetAndMetadata.equals(consumer.committed(Set(tp).asJava).get(tp)), "cannot read committed offset")
}
- @Test(expected = classOf[TimeoutException])
+ @Test
def testInitTransactionsTimeout(): Unit = {
testTimeout(false, producer => producer.initTransactions())
}
- @Test(expected = classOf[TimeoutException])
+ @Test
def testSendOffsetsToTransactionTimeout(): Unit = {
testTimeout(true, producer => producer.sendOffsetsToTransaction(
Map(new TopicPartition(topic1, 0) -> new OffsetAndMetadata(0)).asJava, "test-group"))
}
- @Test(expected = classOf[TimeoutException])
+ @Test
def testCommitTransactionTimeout(): Unit = {
testTimeout(true, producer => producer.commitTransaction())
}
- @Test(expected = classOf[TimeoutException])
+ @Test
def testAbortTransactionTimeout(): Unit = {
testTimeout(true, producer => producer.abortTransaction())
}
- def testTimeout(needInitAndSendMsg: Boolean,
+ private def testTimeout(needInitAndSendMsg: Boolean,
timeoutProcess: KafkaProducer[Array[Byte], Array[Byte]] => Unit): Unit = {
- val producer = createTransactionalProducer("transactionProducer", maxBlockMs = 1000)
-
+ val producer = createTransactionalProducer("transactionProducer", maxBlockMs = 3000)
Review comment:
```1000``` is too small to complete ```createTransactionalProducer``` and it causes ```TimeoutException```. I don't think those test cases tried to test timeout on ```createTransactionalProducer```.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-756297605
@chia7712 Can you please remove the last commit? It seems to have a mix of reverts and it's triggering changes to files I had already reviewed.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r553437994
##########
File path: core/src/test/scala/unit/kafka/utils/QuotaUtilsTest.scala
##########
@@ -17,24 +17,21 @@
package kafka.utils
-import java.util.concurrent.TimeUnit
-
import org.apache.kafka.common.MetricName
-import org.apache.kafka.common.metrics.{KafkaMetric, MetricConfig, Quota, QuotaViolationException}
import org.apache.kafka.common.metrics.stats.{Rate, Value}
-
-import scala.jdk.CollectionConverters._
+import org.apache.kafka.common.metrics.{KafkaMetric, MetricConfig, Quota, QuotaViolationException}
import org.junit.Assert._
import org.junit.Test
-import org.scalatest.Assertions.assertThrows
+
+import java.util.concurrent.TimeUnit
class QuotaUtilsTest {
private val time = new MockTime
private val numSamples = 10
private val sampleWindowSec = 1
private val maxThrottleTimeMs = 500
- private val metricName = new MetricName("test-metric", "groupA", "testA", Map.empty.asJava)
+ private val metricName = new MetricName("test-metric", "groupA", "testA", java.util.Collections.emptyMap())
Review comment:
I would ignore IntelliJ issues if it compiles without warnings. I think there are a few more changes like this in the middle of thousands of lines of diff. Something to avoid in the future. :)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r539416139
##########
File path: clients/src/test/java/org/apache/kafka/clients/NetworkClientTest.java
##########
@@ -117,13 +118,12 @@ public void setup() {
selector.reset();
}
- @Test(expected = IllegalStateException.class)
+ @Test
public void testSendToUnreadyNode() {
MetadataRequest.Builder builder = new MetadataRequest.Builder(Collections.singletonList("test"), true);
long now = time.milliseconds();
ClientRequest request = client.newClientRequest("5", builder, now, false);
- client.send(request, now);
- client.poll(1, time.milliseconds());
Review comment:
We don't need this anymore?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r553386104
##########
File path: core/src/test/scala/unit/kafka/zookeeper/ZooKeeperClientTest.scala
##########
@@ -715,7 +711,7 @@ class ZooKeeperClientTest extends ZooKeeperTestHarness {
private def cleanMetricsRegistry(): Unit = {
val metrics = KafkaYammerMetrics.defaultRegistry
- metrics.allMetrics.keySet.forEach(metrics.removeMetric)
+ metrics.allMetrics.keySet.forEach(m => metrics.removeMetric(m))
Review comment:
IntelliJ can't be aware that lambda function :(
Will revert it.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-751954761
@ijuma Thanks for your great reviewing. I have addressed most review comment in latest commit.
> It would be good to clearly mention the cases where the translation wasn't mechanism as they are the most likely sources of issues.
Pardon me. I failed to catch your point.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r549568729
##########
File path: clients/src/test/java/org/apache/kafka/common/record/MemoryRecordsBuilderTest.java
##########
@@ -504,20 +510,21 @@ public void writePastLimit() {
}
}
- @Test(expected = IllegalArgumentException.class)
+ @Test
public void testAppendAtInvalidOffset() {
ByteBuffer buffer = ByteBuffer.allocate(1024);
buffer.position(bufferOffset);
long logAppendTime = System.currentTimeMillis();
- MemoryRecordsBuilder builder = new MemoryRecordsBuilder(buffer, RecordBatch.MAGIC_VALUE_V1, compressionType,
+ MemoryRecordsBuilder builder = new MemoryRecordsBuilder(buffer, RecordBatch.MAGIC_VALUE_V2, compressionType,
Review comment:
magic 1 does not support ```ZStandard```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-750355111
> Feel free to take one or more of the other ones. core would be a good candidate perhaps.
I’d like to take over both core module and stream module :)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] ijuma edited a comment on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
ijuma edited a comment on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-750345209
@chia7712 Thanks. I was about halfway through the review of the second to last commit. :) Let's get this PR over the line and not change it further please. Yes, I had filed a few JIRAs for migrating to JUnit 5, see https://issues.apache.org/jira/browse/KAFKA-7339.
I think we should do it on a module per module basis to make it easier to review (the JIRAs are structured in that way). I was going to try the clients module after this PR is merged. Feel free to take one or more of the other ones. `core` would be a good candidate perhaps.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#discussion_r546580833
##########
File path: core/src/test/scala/integration/kafka/api/EndToEndAuthorizationTest.scala
##########
@@ -411,14 +411,13 @@ abstract class EndToEndAuthorizationTest extends IntegrationTestHarness with Sas
* Tests that a consumer fails to consume messages without the appropriate
* ACL set.
*/
- @Test(expected = classOf[KafkaException])
+ @Test
def testNoConsumeWithoutDescribeAclViaAssign(): Unit = {
noConsumeWithoutDescribeAclSetup()
val consumer = createConsumer()
consumer.assign(List(tp).asJava)
// the exception is expected when the consumer attempts to lookup offsets
- consumeRecords(consumer)
- confirmReauthenticationMetrics()
Review comment:
good catching!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 edited a comment on pull request #9520: MINOR: replace test "expected" parameter by assertThrows
Posted by GitBox <gi...@apache.org>.
chia7712 edited a comment on pull request #9520:
URL: https://github.com/apache/kafka/pull/9520#issuecomment-748854295
> One more thing: we seem to sometimes use the scalatest asserts and sometimes the JUnit ones for the Scala code. I would suggest using the JUnit ones consistently as it will make it easier to convert the tests to JUnit 5.
Except for ```org.scalatest.Assertions.withClue```, others package from ```org.scalatest.Assertions``` are replaced by Junit. There are other tests using ```scalatest``` and I can remove ```scalatest``` from code base in follow-up.
----------------------------------------------------------------
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:
users@infra.apache.org