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/10 09:04:09 UTC
[GitHub] [flink] snuyanzin opened a new pull request, #19928: [FLINK-27937][tests][pubsub] Migrate flink-connectors-gcp-pubsub to JUnit 5
snuyanzin opened a new pull request, #19928:
URL: https://github.com/apache/flink/pull/19928
## What is the purpose of the change
Migrate flink-connectors-gcp-pubsub module to AssertJ and JUnit 5 following the [JUnit 5 Migration Guide](https://docs.google.com/document/d/1514Wa_aNB9bJUen4xm5uiuXOooOJTtXqS_Jqk9KJitU/edit)
## Brief change log
1. Use JUnit5 and AssertJ in tests instead of JUnit4 and Hamcrest
2. Add dependency on `mockito-junit-jupiter` since pubsub connector requires it
## 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): (yes)
- 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 #19928: [FLINK-27937][tests][pubsub] Migrate flink-connectors-gcp-pubsub to JUnit 5
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on PR #19928:
URL: https://github.com/apache/flink/pull/19928#issuecomment-1152266997
@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 #19928: [FLINK-27937][tests][pubsub] Migrate flink-connectors-gcp-pubsub to JUnit 5
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #19928:
URL: https://github.com/apache/flink/pull/19928#discussion_r965880237
##########
flink-connectors/flink-connector-gcp-pubsub/src/test/java/org/apache/flink/streaming/connectors/gcp/pubsub/PubSubSourceTest.java:
##########
@@ -66,13 +65,17 @@ public class PubSubSourceTest {
private PubSubSource<String> pubSubSource;
- @Before
- public void setup() throws Exception {
- when(pubSubSubscriberFactory.getSubscriber(eq(credentials))).thenReturn(pubsubSubscriber);
- when(streamingRuntimeContext.isCheckpointingEnabled()).thenReturn(true);
- when(streamingRuntimeContext.getMetricGroup()).thenReturn(metricGroup);
- when(metricGroup.addGroup(any(String.class))).thenReturn(metricGroup);
- when(acknowledgeOnCheckpointFactory.create(any())).thenReturn(acknowledgeOnCheckpoint);
+ @BeforeEach
+ void setup() throws Exception {
+ lenient()
+ .when(pubSubSubscriberFactory.getSubscriber(eq(credentials)))
+ .thenReturn(pubsubSubscriber);
+ lenient().when(streamingRuntimeContext.isCheckpointingEnabled()).thenReturn(true);
+ lenient().when(streamingRuntimeContext.getMetricGroup()).thenReturn(metricGroup);
+ lenient().when(metricGroup.addGroup(any(String.class))).thenReturn(metricGroup);
+ lenient()
+ .when(acknowledgeOnCheckpointFactory.create(any()))
+ .thenReturn(acknowledgeOnCheckpoint);
Review Comment:
It looks like these stubbing methods are not always required for all the tests
e.g. `when(pubSubSubscriberFactory.getSubscriber(eq(credentials)))
.thenReturn(pubsubSubscriber)` is required only for `testOpenWithCheckpointing`
for others it fails with `Unnecessary stubbings detected.`
Similar for others. It is strange to me why it didn't fail like that with JUnit4.
Probably another approach it to move these stubbing to concrete methods where to leave only required
--
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 #19928: [FLINK-27937][tests][pubsub] Migrate flink-connectors-gcp-pubsub to JUnit 5
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #19928:
URL: https://github.com/apache/flink/pull/19928#discussion_r967316371
##########
flink-connectors/flink-connector-gcp-pubsub/src/test/java/org/apache/flink/streaming/connectors/gcp/pubsub/PubSubSourceTest.java:
##########
@@ -66,13 +65,17 @@ public class PubSubSourceTest {
private PubSubSource<String> pubSubSource;
- @Before
- public void setup() throws Exception {
- when(pubSubSubscriberFactory.getSubscriber(eq(credentials))).thenReturn(pubsubSubscriber);
- when(streamingRuntimeContext.isCheckpointingEnabled()).thenReturn(true);
- when(streamingRuntimeContext.getMetricGroup()).thenReturn(metricGroup);
- when(metricGroup.addGroup(any(String.class))).thenReturn(metricGroup);
- when(acknowledgeOnCheckpointFactory.create(any())).thenReturn(acknowledgeOnCheckpoint);
+ @BeforeEach
+ void setup() throws Exception {
+ lenient()
+ .when(pubSubSubscriberFactory.getSubscriber(eq(credentials)))
+ .thenReturn(pubsubSubscriber);
+ lenient().when(streamingRuntimeContext.isCheckpointingEnabled()).thenReturn(true);
+ lenient().when(streamingRuntimeContext.getMetricGroup()).thenReturn(metricGroup);
+ lenient().when(metricGroup.addGroup(any(String.class))).thenReturn(metricGroup);
+ lenient()
+ .when(acknowledgeOnCheckpointFactory.create(any()))
+ .thenReturn(acknowledgeOnCheckpoint);
Review Comment:
After some research it seems that in case of JUnit5 Mockito works a bit different in compare how it works for JUnit4.
The reason is that for JUnit5 there `org.mockito.junit.jupiter.MockitoExtension` [1] is used
and for JUnit4 `org.mockito.junit.MockitoJUnitRunner` [2].
The way how validation of used stubs is working is also different.
E.g. in case of JUnit5 it is handled at `org.mockito.MockitoSession#finishMocking(java.lang.Throwable)` [3] which is invoked after each test.
At the same time in JUnit4 it is processed inside `org.mockito.internal.runners.StrictRunner#run` [4] and logic there is the next one:
first it works only for cases when all tests from class/testsuite are running
second it does validation of used/not used stubs after all tests finished.
Seems it is the reason why after movement to JUnit5 it starts failing
[1] https://github.com/mockito/mockito/blob/v3.4.6/subprojects/junit-jupiter/src/main/java/org/mockito/junit/jupiter/MockitoExtension.java
[2] https://github.com/mockito/mockito/blob/v3.4.6/src/main/java/org/mockito/junit/MockitoJUnitRunner.java
[3] https://github.com/mockito/mockito/blob/v3.4.6/subprojects/junit-jupiter/src/main/java/org/mockito/junit/jupiter/MockitoExtension.java#L184-L186
[4] https://github.com/mockito/mockito/blob/v3.4.6/src/main/java/org/mockito/internal/runners/StrictRunner.java#L45-L53
--
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 #19928: [FLINK-27937][tests][pubsub] Migrate flink-connectors-gcp-pubsub to JUnit 5
Posted by GitBox <gi...@apache.org>.
XComp commented on code in PR #19928:
URL: https://github.com/apache/flink/pull/19928#discussion_r962847108
##########
flink-connectors/flink-connector-gcp-pubsub/src/test/java/org/apache/flink/streaming/connectors/gcp/pubsub/PubSubSourceTest.java:
##########
@@ -66,13 +65,17 @@ public class PubSubSourceTest {
private PubSubSource<String> pubSubSource;
- @Before
- public void setup() throws Exception {
- when(pubSubSubscriberFactory.getSubscriber(eq(credentials))).thenReturn(pubsubSubscriber);
- when(streamingRuntimeContext.isCheckpointingEnabled()).thenReturn(true);
- when(streamingRuntimeContext.getMetricGroup()).thenReturn(metricGroup);
- when(metricGroup.addGroup(any(String.class))).thenReturn(metricGroup);
- when(acknowledgeOnCheckpointFactory.create(any())).thenReturn(acknowledgeOnCheckpoint);
+ @BeforeEach
+ void setup() throws Exception {
+ lenient()
+ .when(pubSubSubscriberFactory.getSubscriber(eq(credentials)))
+ .thenReturn(pubsubSubscriber);
+ lenient().when(streamingRuntimeContext.isCheckpointingEnabled()).thenReturn(true);
+ lenient().when(streamingRuntimeContext.getMetricGroup()).thenReturn(metricGroup);
+ lenient().when(metricGroup.addGroup(any(String.class))).thenReturn(metricGroup);
+ lenient()
+ .when(acknowledgeOnCheckpointFactory.create(any()))
+ .thenReturn(acknowledgeOnCheckpoint);
Review Comment:
I know that there's a bit of time passed since you touched this PR. But can you elaborate a bit on why we're switching to `lenient` here rather than sticking to the old approach. My understanding is that `lenient()` relaxes the test condition a bit?
--
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 #19928: [FLINK-27937][tests][pubsub] Migrate flink-connectors-gcp-pubsub to JUnit 5
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #19928:
URL: https://github.com/apache/flink/pull/19928#issuecomment-1152148511
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "37e4f95c27ab4bcc1d48f30a26e8a074d84c502b",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "37e4f95c27ab4bcc1d48f30a26e8a074d84c502b",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 37e4f95c27ab4bcc1d48f30a26e8a074d84c502b 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
[GitHub] [flink] snuyanzin commented on a diff in pull request #19928: [FLINK-27937][tests][pubsub] Migrate flink-connectors-gcp-pubsub to JUnit 5
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #19928:
URL: https://github.com/apache/flink/pull/19928#discussion_r965880237
##########
flink-connectors/flink-connector-gcp-pubsub/src/test/java/org/apache/flink/streaming/connectors/gcp/pubsub/PubSubSourceTest.java:
##########
@@ -66,13 +65,17 @@ public class PubSubSourceTest {
private PubSubSource<String> pubSubSource;
- @Before
- public void setup() throws Exception {
- when(pubSubSubscriberFactory.getSubscriber(eq(credentials))).thenReturn(pubsubSubscriber);
- when(streamingRuntimeContext.isCheckpointingEnabled()).thenReturn(true);
- when(streamingRuntimeContext.getMetricGroup()).thenReturn(metricGroup);
- when(metricGroup.addGroup(any(String.class))).thenReturn(metricGroup);
- when(acknowledgeOnCheckpointFactory.create(any())).thenReturn(acknowledgeOnCheckpoint);
+ @BeforeEach
+ void setup() throws Exception {
+ lenient()
+ .when(pubSubSubscriberFactory.getSubscriber(eq(credentials)))
+ .thenReturn(pubsubSubscriber);
+ lenient().when(streamingRuntimeContext.isCheckpointingEnabled()).thenReturn(true);
+ lenient().when(streamingRuntimeContext.getMetricGroup()).thenReturn(metricGroup);
+ lenient().when(metricGroup.addGroup(any(String.class))).thenReturn(metricGroup);
+ lenient()
+ .when(acknowledgeOnCheckpointFactory.create(any()))
+ .thenReturn(acknowledgeOnCheckpoint);
Review Comment:
It looks like these stubbing methods are not always required for all the tests
e.g. `when(pubSubSubscriberFactory.getSubscriber(eq(credentials)))
.thenReturn(pubsubSubscriber)` is required only for `testOpenWithCheckpointing`
for others it fails with `Unnecessary stubbings detected.`
Similar for others. It is strange to me why it didn't fail like that with JUnit4
--
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 #19928: [FLINK-27937][tests][pubsub] Migrate flink-connectors-gcp-pubsub to JUnit 5
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #19928:
URL: https://github.com/apache/flink/pull/19928#discussion_r965880237
##########
flink-connectors/flink-connector-gcp-pubsub/src/test/java/org/apache/flink/streaming/connectors/gcp/pubsub/PubSubSourceTest.java:
##########
@@ -66,13 +65,17 @@ public class PubSubSourceTest {
private PubSubSource<String> pubSubSource;
- @Before
- public void setup() throws Exception {
- when(pubSubSubscriberFactory.getSubscriber(eq(credentials))).thenReturn(pubsubSubscriber);
- when(streamingRuntimeContext.isCheckpointingEnabled()).thenReturn(true);
- when(streamingRuntimeContext.getMetricGroup()).thenReturn(metricGroup);
- when(metricGroup.addGroup(any(String.class))).thenReturn(metricGroup);
- when(acknowledgeOnCheckpointFactory.create(any())).thenReturn(acknowledgeOnCheckpoint);
+ @BeforeEach
+ void setup() throws Exception {
+ lenient()
+ .when(pubSubSubscriberFactory.getSubscriber(eq(credentials)))
+ .thenReturn(pubsubSubscriber);
+ lenient().when(streamingRuntimeContext.isCheckpointingEnabled()).thenReturn(true);
+ lenient().when(streamingRuntimeContext.getMetricGroup()).thenReturn(metricGroup);
+ lenient().when(metricGroup.addGroup(any(String.class))).thenReturn(metricGroup);
+ lenient()
+ .when(acknowledgeOnCheckpointFactory.create(any()))
+ .thenReturn(acknowledgeOnCheckpoint);
Review Comment:
It looks like these stubbing methods are not always required for all the tests
e.g. `when(pubSubSubscriberFactory.getSubscriber(eq(credentials)))
.thenReturn(pubsubSubscriber)` is required only for `testOpenWithCheckpointing`
for others it fails with `Unnecessary stubbings detected.`
Similar for others. And `lenient()` allows to skip such checks as also mentioned in their javadoc [1]
It is strange to me why it didn't fail like that with JUnit4.
Probably another approach it to move these stubbing to concrete methods where to leave only required
[1] https://github.com/mockito/mockito/blob/main/src/main/java/org/mockito/Mockito.java#L3307-L3309
--
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 #19928: [FLINK-27937][tests][pubsub] Migrate flink-connectors-gcp-pubsub to JUnit 5
Posted by GitBox <gi...@apache.org>.
XComp commented on code in PR #19928:
URL: https://github.com/apache/flink/pull/19928#discussion_r968347335
##########
flink-connectors/flink-connector-gcp-pubsub/src/test/java/org/apache/flink/streaming/connectors/gcp/pubsub/PubSubSourceTest.java:
##########
@@ -66,13 +65,17 @@ public class PubSubSourceTest {
private PubSubSource<String> pubSubSource;
- @Before
- public void setup() throws Exception {
- when(pubSubSubscriberFactory.getSubscriber(eq(credentials))).thenReturn(pubsubSubscriber);
- when(streamingRuntimeContext.isCheckpointingEnabled()).thenReturn(true);
- when(streamingRuntimeContext.getMetricGroup()).thenReturn(metricGroup);
- when(metricGroup.addGroup(any(String.class))).thenReturn(metricGroup);
- when(acknowledgeOnCheckpointFactory.create(any())).thenReturn(acknowledgeOnCheckpoint);
+ @BeforeEach
+ void setup() throws Exception {
+ lenient()
+ .when(pubSubSubscriberFactory.getSubscriber(eq(credentials)))
+ .thenReturn(pubsubSubscriber);
+ lenient().when(streamingRuntimeContext.isCheckpointingEnabled()).thenReturn(true);
+ lenient().when(streamingRuntimeContext.getMetricGroup()).thenReturn(metricGroup);
+ lenient().when(metricGroup.addGroup(any(String.class))).thenReturn(metricGroup);
+ lenient()
+ .when(acknowledgeOnCheckpointFactory.create(any()))
+ .thenReturn(acknowledgeOnCheckpoint);
Review Comment:
Thanks for clarification, @snuyanzin
--
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 merged pull request #19928: [FLINK-27937][tests][pubsub] Migrate flink-connectors-gcp-pubsub to JUnit 5
Posted by GitBox <gi...@apache.org>.
XComp merged PR #19928:
URL: https://github.com/apache/flink/pull/19928
--
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