[FLINK-25538][Connectors/Kafka] Migration flink-connector-kafka from Junit4 to Junit5 #106
Conversation
|
Thanks for opening this pull request! Please check out our contributing guidelines. (https://flink.apache.org/contributing/how-to-contribute.html) |
|
Thanks for your contribution and driving this activity. |
|
Hey @snuyanzin, thanks for the feedback! I did quick grep, on the previous PR (#66) it is 398 tests, and in this PR it is 429. Current PR is already main rebased so expected more tests. I couldn't run the whole build locally yet, but I'll check again once I am successful. |
|
|
||
| /** Tests for {@link KafkaRecordSerializationSchemaBuilder}. */ | ||
| public class KafkaRecordSerializationSchemaBuilderTest extends TestLogger { | ||
| @ExtendWith({TestLoggerExtension.class}) |
There was a problem hiding this comment.
IIRC we don't need to put each time this ExtendsWith for almost every test
instead just put it once in services (common practice in different flink modules) like e.g. here
https://github.com/apache/flink/blob/e3b123d7d1e48e7adbb04fb3470d02bb76f5ff73/flink-formats/flink-avro-confluent-registry/src/test/resources/META-INF/services/org.junit.jupiter.api.extension.Extension#L16
| assertThat(firstProducer != writer.getCurrentProducer()) | ||
| .as("Expected different producer") | ||
| .isTrue(); | ||
| .isNotSameAs(writer.getCurrentProducer()); |
There was a problem hiding this comment.
seems like a wrong statement which always will pass the test since firstProducer != writer.getCurrentProducer() is always is not same as writer.getCurrentProducer()
I guess the correct one should be
assertThat(firstProducer)
.as("Expected different producer")
.isNotSameAs(writer.getCurrentProducer());
| assertThatThrownBy(() -> testHarness.processElement(new StreamRecord<>("msg-2"))) | ||
| .hasStackTraceContaining("artificial async exception"); |
There was a problem hiding this comment.
Here it is not one-to-one migration since it's started to check the, whole trace instead of just a cause
| fail("unknown failure"); | ||
| // the next invoke should rethrow the async exception | ||
| assertThatThrownBy(() -> testHarness.snapshot(123L, 123L)) | ||
| .hasStackTraceContaining("artificial async exception"); |
| fail("unknown failure"); | ||
| // the snapshot should have failed with the async exception | ||
| assertThatThrownBy(() -> snapshotThread.sync()) | ||
| .hasStackTraceContaining("artificial async failure for 2nd message"); |
|
Hello I am new to the project and want to start somewhere. Is this still being worked on? If not I can take over the changes |
|
Hey @amitvc , Great, thanks! Please go ahead and take the task(s)! I couldn't find time to work on these recently, let me know if you need anything 🤝 |
|
morazow Thank you for your reply. I will take this over. |
What is the purpose of the change
Brief change log
Verifying this change
Documentation