Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ISSUE#4510] Add test case for SpringSinkConnector. #4511

Merged
merged 4 commits into from
Oct 26, 2023

Conversation

yanrongzhen
Copy link
Contributor

Fixes #4510 .

Modifications

  • Add test case for SpringSinkConnector.
  • Use the Version Catalog mechanism for hamcrest versioning.

@harshithasudhakar
Copy link
Member

LGTM, but can we have a more descriptive method name? Like shouldProcessRecordsInQueue? This would make the test's purpose clearer. Additionally, can we consider adding another test case to handle an empty queue? Testing for this scenario is important to ensure the connector's behavior when the queue is empty is as expected. Including edge cases empty queue, can help improve code coverage in test suite.

ConnectRecord poll = queue.poll();
assertNotNull(poll);
String expectedMessage = message + i;
assertThat(poll.getData(), is(expectedMessage));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it more convenient to use JUnit 5's Assertions directly? Why did you deliberately introduce 'hamcrest' to make assertions?


直接使用JUnit 5的Assertions不是更方便吗?为何特意引入'hamcrest'来断言?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hamcrest's assertion is more powerful than junit5, and I think can be combined.


hamcrest的断言比junit5更强大, 我认为两者可以组合使用。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But your test case does not require more powerful features in 'hamcrest' than 'JUnit'. Shouldn't it be used on demand?


但是你的测试用例并不需要'hamcrest'比'JUnit'更强大的特性啊。不应该按需使用吗?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hamcrest is an additional dependency, and if we're using it solely for simple assertions, I believe that standard JUnit assertions should suffice. This can help us keep our project dependencies minimal and reduce complexity in our tests. This is just my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it's right to keep minimal dependencies, it has been fixed in new commit.

@yanrongzhen
Copy link
Contributor Author

LGTM, but can we have a more descriptive method name? Like shouldProcessRecordsInQueue? This would make the test's purpose clearer. Additionally, can we consider adding another test case to handle an empty queue? Testing for this scenario is important to ensure the connector's behavior when the queue is empty is as expected. Including edge cases empty queue, can help improve code coverage in test suite.

The name of the unit test method has been changed, but about the unit test when the queue is empty, the original class does not have a special case, so i think it does not need to be handle with.

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #4511 (97ef124) into master (cf16cf6) will increase coverage by 0.05%.
The diff coverage is n/a.

❗ Current head 97ef124 differs from pull request most recent head e009a26. Consider uploading reports for the commit e009a26 to get more accurate results

@@             Coverage Diff              @@
##             master    #4511      +/-   ##
============================================
+ Coverage     15.92%   15.97%   +0.05%     
- Complexity     1541     1549       +8     
============================================
  Files           727      727              
  Lines         28872    28872              
  Branches       2744     2744              
============================================
+ Hits           4597     4613      +16     
+ Misses        23792    23776      -16     
  Partials        483      483              

see 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pandaapo pandaapo merged commit ae76d75 into apache:master Oct 26, 2023
7 checks passed
Copy link
Member

@Pil0tXia Pil0tXia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xwm1992 xwm1992 added this to the 1.10 milestone Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] Add unit test for SpringSinkConnector.
5 participants