You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/06/08 18:07:24 UTC
[GitHub] [flink] snuyanzin opened a new pull request, #19914: [FLINK-27942][tests] Migrate flink-connectors-rabbitmq to JUnit5
snuyanzin opened a new pull request, #19914:
URL: https://github.com/apache/flink/pull/19914
## What is the purpose of the change
1. Migrate flink-connectors-rabbitmq module to AssertJ and JUnit 5 following the [JUnit 5 Migration Guide](https://docs.google.com/document/d/1514Wa_aNB9bJUen4xm5uiuXOooOJTtXqS_Jqk9KJitU/edit)
2. Fix RestClusterClient injection
## Brief change log
Use JUnit5 and AssertJ in tests instead of JUnit4 and Hamcrest
## Verifying this change
This change is a code cleanup without any test coverage.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no)
- The serializers: (no)
- The runtime per-record code paths (performance sensitive): (no)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no )
- The S3 file system connector: (no )
## Documentation
- Does this pull request introduce a new feature? (no )
- If yes, how is the feature documented? (not applicable)
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] snuyanzin commented on pull request #19914: [FLINK-27942][tests] Migrate flink-connectors-rabbitmq to JUnit5
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on PR #19914:
URL: https://github.com/apache/flink/pull/19914#issuecomment-1150663562
@flinkbot run azure
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #19914: [FLINK-27942][tests] Migrate flink-connectors-rabbitmq to JUnit5
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #19914:
URL: https://github.com/apache/flink/pull/19914#discussion_r892709873
##########
flink-test-utils-parent/flink-test-utils/src/main/java/org/apache/flink/test/junit5/MiniClusterExtension.java:
##########
@@ -170,7 +170,8 @@ public boolean supportsParameter(
throws ParameterResolutionException {
Class<?> parameterType = parameterContext.getParameter().getType();
if (parameterContext.isAnnotated(InjectClusterClient.class)
- && parameterType.isAssignableFrom(ClusterClient.class)) {
+ && (parameterType.isAssignableFrom(ClusterClient.class)
+ || parameterType.isAssignableFrom(RestClusterClient.class))) {
Review Comment:
To allow `RestClusterClient` injection in tests
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] XComp commented on a diff in pull request #19914: [FLINK-27942][tests] Migrate flink-connectors-rabbitmq to JUnit5
Posted by GitBox <gi...@apache.org>.
XComp commented on code in PR #19914:
URL: https://github.com/apache/flink/pull/19914#discussion_r983676804
##########
flink-connectors/flink-connector-rabbitmq/src/test/java/org/apache/flink/streaming/connectors/rabbitmq/RMQSourceITCase.java:
##########
@@ -112,7 +117,8 @@ public void setUp() throws Exception {
}
@Test
- public void testStopWithSavepoint() throws Exception {
+ void testStopWithSavepoint(@InjectClusterClient RestClusterClient<?> clusterClient)
Review Comment:
```suggestion
void testStopWithSavepoint(@InjectClusterClient RestClusterClient<?> clusterClient, @TempDir Path tmp)
```
nit: We could also just path the temporary directory into this method instead of having it as a field variable. ...since it's the only location where the temporary directory is used. But that's just personal taste, I guess. :thinking:
##########
flink-connectors/flink-connector-rabbitmq/src/test/java/org/apache/flink/streaming/connectors/rabbitmq/common/RMQConnectionConfigTest.java:
##########
@@ -18,63 +18,74 @@
package org.apache.flink.streaming.connectors.rabbitmq.common;
import com.rabbitmq.client.ConnectionFactory;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
import java.net.URISyntaxException;
import java.security.KeyManagementException;
import java.security.NoSuchAlgorithmException;
import java.util.Optional;
import java.util.concurrent.TimeUnit;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
/** Tests for the {@link RMQConnectionConfig}. */
-public class RMQConnectionConfigTest {
+class RMQConnectionConfigTest {
- @Test(expected = NullPointerException.class)
- public void shouldThrowNullPointExceptionIfHostIsNull()
- throws NoSuchAlgorithmException, KeyManagementException, URISyntaxException {
- RMQConnectionConfig connectionConfig =
- new RMQConnectionConfig.Builder()
- .setPort(1000)
- .setUserName("guest")
- .setPassword("guest")
- .setVirtualHost("/")
- .build();
- connectionConfig.getConnectionFactory();
+ @Test
+ void shouldThrowNullPointExceptionIfHostIsNull() {
+ assertThatThrownBy(
+ () -> {
+ RMQConnectionConfig connectionConfig =
+ new RMQConnectionConfig.Builder()
+ .setPort(1000)
+ .setUserName("guest")
+ .setPassword("guest")
+ .setVirtualHost("/")
+ .build();
+ connectionConfig.getConnectionFactory();
+ })
+ .isInstanceOf(NullPointerException.class);
}
- @Test(expected = NullPointerException.class)
- public void shouldThrowNullPointExceptionIfPortIsNull()
+ @Test
+ void shouldThrowNullPointExceptionIfPortIsNull()
throws NoSuchAlgorithmException, KeyManagementException, URISyntaxException {
Review Comment:
```suggestion
```
unused
##########
flink-connectors/flink-connector-rabbitmq/src/test/java/org/apache/flink/streaming/connectors/rabbitmq/RMQSourceITCase.java:
##########
@@ -112,7 +117,8 @@ public void setUp() throws Exception {
}
@Test
- public void testStopWithSavepoint() throws Exception {
+ void testStopWithSavepoint(@InjectClusterClient RestClusterClient<?> clusterClient)
Review Comment:
```suggestion
void testStopWithSavepoint(@InjectClusterClient RestClusterClient<?> clusterClient, @TempDir Path tmp)
```
nit: we could inject the `@TempDir` as a parameter since it's only used once in the test class.
##########
flink-connectors/flink-connector-rabbitmq/src/test/java/org/apache/flink/streaming/connectors/rabbitmq/RMQSourceTest.java:
##########
@@ -423,37 +418,38 @@ public void testConstructorParams() throws Exception {
// connection fails but check if args have been passed correctly
}
- assertEquals("hostTest", testObj.getFactory().getHost());
- assertEquals(999, testObj.getFactory().getPort());
- assertEquals("userTest", testObj.getFactory().getUsername());
- assertEquals("passTest", testObj.getFactory().getPassword());
+ assertThat(testObj.getFactory().getHost()).isEqualTo("hostTest");
+ assertThat(testObj.getFactory().getPort()).isEqualTo(999);
+ assertThat(testObj.getFactory().getUsername()).isEqualTo("userTest");
+ assertThat(testObj.getFactory().getPassword()).isEqualTo("passTest");
}
- @Test(timeout = 30000L)
- public void testCustomIdentifiers() throws Exception {
+ @Test
+ @Timeout(30)
Review Comment:
```suggestion
@Timeout(value = 30_000, unit = TimeUnit.MILLISECONDS)
```
I feel like adding the time unit here helps the readability. Especially, because readers might be more used to using milliseconds as a timeout unit. `@Timeout(30)` might leave the wrong impression. WDYT?
##########
flink-connectors/flink-connector-rabbitmq/src/test/java/org/apache/flink/streaming/connectors/rabbitmq/common/RMQConnectionConfigTest.java:
##########
@@ -18,63 +18,74 @@
package org.apache.flink.streaming.connectors.rabbitmq.common;
import com.rabbitmq.client.ConnectionFactory;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
import java.net.URISyntaxException;
import java.security.KeyManagementException;
import java.security.NoSuchAlgorithmException;
import java.util.Optional;
import java.util.concurrent.TimeUnit;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
/** Tests for the {@link RMQConnectionConfig}. */
-public class RMQConnectionConfigTest {
+class RMQConnectionConfigTest {
- @Test(expected = NullPointerException.class)
- public void shouldThrowNullPointExceptionIfHostIsNull()
- throws NoSuchAlgorithmException, KeyManagementException, URISyntaxException {
- RMQConnectionConfig connectionConfig =
- new RMQConnectionConfig.Builder()
- .setPort(1000)
- .setUserName("guest")
- .setPassword("guest")
- .setVirtualHost("/")
- .build();
- connectionConfig.getConnectionFactory();
+ @Test
+ void shouldThrowNullPointExceptionIfHostIsNull() {
+ assertThatThrownBy(
+ () -> {
+ RMQConnectionConfig connectionConfig =
+ new RMQConnectionConfig.Builder()
+ .setPort(1000)
+ .setUserName("guest")
+ .setPassword("guest")
+ .setVirtualHost("/")
+ .build();
+ connectionConfig.getConnectionFactory();
+ })
+ .isInstanceOf(NullPointerException.class);
}
- @Test(expected = NullPointerException.class)
- public void shouldThrowNullPointExceptionIfPortIsNull()
+ @Test
+ void shouldThrowNullPointExceptionIfPortIsNull()
throws NoSuchAlgorithmException, KeyManagementException, URISyntaxException {
- RMQConnectionConfig connectionConfig =
- new RMQConnectionConfig.Builder()
- .setHost("localhost")
- .setUserName("guest")
- .setPassword("guest")
- .setVirtualHost("/")
- .build();
- connectionConfig.getConnectionFactory();
+ assertThatThrownBy(
+ () -> {
+ RMQConnectionConfig connectionConfig =
+ new RMQConnectionConfig.Builder()
+ .setHost("localhost")
+ .setUserName("guest")
+ .setPassword("guest")
+ .setVirtualHost("/")
+ .build();
+ connectionConfig.getConnectionFactory();
+ })
+ .isInstanceOf(NullPointerException.class);
}
- @Test(expected = NullPointerException.class)
- public void shouldSetDefaultValueIfConnectionTimeoutNotGiven()
+ @Test
+ void shouldSetDefaultValueIfConnectionTimeoutNotGiven()
throws NoSuchAlgorithmException, KeyManagementException, URISyntaxException {
Review Comment:
```suggestion
```
unused
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #19914: [FLINK-27942][tests] Migrate flink-connectors-rabbitmq to JUnit5
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #19914:
URL: https://github.com/apache/flink/pull/19914#issuecomment-1150234202
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f46c06ded4dd8f39f6d5c16a7aadba6e4749aefb",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "f46c06ded4dd8f39f6d5c16a7aadba6e4749aefb",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* f46c06ded4dd8f39f6d5c16a7aadba6e4749aefb UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org