You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "C0urante (via GitHub)" <gi...@apache.org> on 2023/02/16 23:42:44 UTC
[GitHub] [kafka] C0urante opened a new pull request, #13266: MINOR: Fix PluginInfoTest for Connect
C0urante opened a new pull request, #13266:
URL: https://github.com/apache/kafka/pull/13266
The refactorings in https://github.com/apache/kafka/pull/13219 are largely very helpful; however, the changes to the `PluginInfoTest` suite have caused the test to begin failing.
This PR reverts the changes to that test suite and adds a note on why we use `assertTrue(foo.equals(bar))` instead of `assertEquals(foo, bar)`.
### 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a diff in pull request #13266: MINOR: Fix PluginInfoTest for Connect
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #13266:
URL: https://github.com/apache/kafka/pull/13266#discussion_r1109343158
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/entities/PluginInfoTest.java:
##########
@@ -19,17 +19,19 @@
import org.apache.kafka.connect.runtime.isolation.DelegatingClassLoader;
import org.junit.Test;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
public class PluginInfoTest {
@Test
public void testNoVersionFilter() {
PluginInfo.NoVersionFilter filter = new PluginInfo.NoVersionFilter();
- assertNotEquals("1.0", filter);
- assertNotEquals(filter, new Object());
- assertNotEquals(null, filter);
- assertEquals(DelegatingClassLoader.UNDEFINED_VERSION, filter);
+ // We intentionally refrain from using assertEquals and assertNotEquals
+ // here to ensure that the filter's equals() method is used
+ assertFalse(filter.equals("1.0"));
Review Comment:
how about using `assertNotEquals(filter, "1.0");`? It can produce more readable failure message.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #13266: MINOR: Fix PluginInfoTest for Connect
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on PR #13266:
URL: https://github.com/apache/kafka/pull/13266#issuecomment-1434809151
Since this is causing build failures on trunk and, with the exception of a two-line comment, the changes revert the test class in question to its last green state, I'm going to merge this change.
I will be happy to address any questions/comments here and, if necessary, file a follow-up 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #13266: MINOR: Fix PluginInfoTest for Connect
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #13266:
URL: https://github.com/apache/kafka/pull/13266#issuecomment-1434850103
My bad, I thought I ran tests on all changed classes but clearly I missed some. Thanks @C0urante for the quick fix!
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on a diff in pull request #13266: MINOR: Fix PluginInfoTest for Connect
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on code in PR #13266:
URL: https://github.com/apache/kafka/pull/13266#discussion_r1109946722
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/entities/PluginInfoTest.java:
##########
@@ -19,17 +19,19 @@
import org.apache.kafka.connect.runtime.isolation.DelegatingClassLoader;
import org.junit.Test;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
public class PluginInfoTest {
@Test
public void testNoVersionFilter() {
PluginInfo.NoVersionFilter filter = new PluginInfo.NoVersionFilter();
- assertNotEquals("1.0", filter);
- assertNotEquals(filter, new Object());
- assertNotEquals(null, filter);
- assertEquals(DelegatingClassLoader.UNDEFINED_VERSION, filter);
+ // We intentionally refrain from using assertEquals and assertNotEquals
+ // here to ensure that the filter's equals() method is used
+ assertFalse(filter.equals("1.0"));
Review Comment:
We intentionally refrain from this as it does not guarantee that the filter's equals method is used. The filter's equals method is what we need to test here since it is used by Jackson to prevent some fields from being deserialized in REST entities depending on their values.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] clolov commented on pull request #13266: MINOR: Fix PluginInfoTest for Connect
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on PR #13266:
URL: https://github.com/apache/kafka/pull/13266#issuecomment-1434334778
Ah, apologies, I wasn't aware of this behaviour
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante merged pull request #13266: MINOR: Fix PluginInfoTest for Connect
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante merged PR #13266:
URL: https://github.com/apache/kafka/pull/13266
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #13266: MINOR: Fix PluginInfoTest for Connect
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on PR #13266:
URL: https://github.com/apache/kafka/pull/13266#issuecomment-1434145501
@chia7712 would you have a moment to take a look at this?
I'm going to bed now; feel free to merge if everything looks alright.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #13266: MINOR: Fix PluginInfoTest for Connect
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on PR #13266:
URL: https://github.com/apache/kafka/pull/13266#issuecomment-1434814552
@clolov No worries :)
It may be worth checking the CI build results for your PRs in the future. I know it can be tricky to tell sometimes if a test failure is due to flakiness or not, but if something fails on every different JDK/Scala version combination, it's probably a real failure and should be addressed before merging.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org