-
Notifications
You must be signed in to change notification settings - Fork 67
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
Replace testng with junit5 for gateway ha module #62
Conversation
Can you squash commits and update message to follow standard format. See https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages |
gateway-ha/pom.xml
Outdated
<artifactId>junit-jupiter-params</artifactId> | ||
<scope>test</scope> | ||
</dependency> | ||
|
||
<dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove testng
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mosabua It will throw testng exception if we remove it at this time. I think it is best to remove testng from pom once we remove all imports. We still use testng in baseapp module, and I'll fix it after this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No .. lets clean it up in this module completely. Remove all imports in this module and if necessary override the surefire config. Lets clean it up for real... that way nobody can accidentally add a testng test in again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok fixed
b246636
to
12b14c8
Compare
I've fixed the commit messages. Let me know if it needs to fixed again |
12b14c8
to
a2da2a3
Compare
<groupId>org.apache.maven.surefire</groupId> | ||
<artifactId>surefire-testng</artifactId> | ||
<version>${dep.plugin.surefire.version}</version> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have to get rid of testng from parent pom otherwise gateway-ha will inherit and throw exception because we removed all testng related deps from gateway-ha
<version>${dep.plugin.surefire.version}</version> | ||
</dependency> | ||
</dependencies> | ||
</plugin> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have to add testng dep to this module because it no longer inherits this from parent pom. Can get rid of this once we migrate this module from testng to junit5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, a few minor comments. The Ordering could be left in and fixed in a separate PR, I just wanted to note it while we're here.
@@ -1,17 +1,21 @@ | |||
package io.trino.gateway.ha; | |||
|
|||
import static org.junit.Assert.assertEquals; | |||
import static org.junit.jupiter.api.TestInstance.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicitly import what is needed, don't use .*
@@ -1,21 +1,28 @@ | |||
package io.trino.gateway.ha.router; | |||
|
|||
import static org.junit.jupiter.api.Assertions.assertEquals; | |||
import static org.junit.jupiter.api.TestInstance.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above - use explicit imports.
|
||
@Test | ||
@TestMethodOrder(MethodOrderer.OrderAnnotation.class) | ||
@TestInstance(Lifecycle.PER_CLASS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These test methods can't be run independently - we should rework them to test from a consistent starting point and use Lifecycle.PER_METHOD
or a @BeforeEach
method to reset the state of the backend DB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be taken care of in another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would be cleaner.
@@ -1,23 +1,26 @@ | |||
package io.trino.gateway.ha.router; | |||
|
|||
import static org.junit.jupiter.api.TestInstance.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.*
@@ -35,6 +42,7 @@ public void setUp() { | |||
} | |||
|
|||
@Test | |||
@Order(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above w.r.t. Ordering
} | ||
|
||
//Todo: The functionality of reading the file before every request needs to be smarter | ||
@Test(enabled = false) | ||
@Disabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was initially disabled. Will have to look into why it was disabled, but I think it's out of the scope of this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, looking again I guess it is related to that threadsafety issue. I have a PR open to fix that, so you can ignore my comment here.
@@ -47,15 +51,15 @@ public void testConnectionCheckerSucess() throws | |||
ConnectionChecker checker = new ConnectionChecker(); | |||
ConnectionCheck check = spy(checker.getChecker("abc", 1111, checkInterval, 1, 0)); | |||
doReturn(mock(Socket.class)).when(check).makeSocket("abc", 1111); | |||
Assert.assertTrue(check.tcpCheck() == ConnectionCheck.TCP_CHECK_SUCCESS); | |||
assertTrue(check.tcpCheck() == ConnectionCheck.TCP_CHECK_SUCCESS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use AssertEquals
and AssertNotEqual
vs using ==
and !=
?
} | ||
|
||
//Todo: The functionality of reading the file before every request needs to be smarter | ||
@Test(enabled = false) | ||
@Disabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be enabled?
@@ -47,15 +51,15 @@ public void testConnectionCheckerSucess() throws | |||
ConnectionChecker checker = new ConnectionChecker(); | |||
ConnectionCheck check = spy(checker.getChecker("abc", 1111, checkInterval, 1, 0)); | |||
doReturn(mock(Socket.class)).when(check).makeSocket("abc", 1111); | |||
Assert.assertTrue(check.tcpCheck() == ConnectionCheck.TCP_CHECK_SUCCESS); | |||
assertTrue(check.tcpCheck() == ConnectionCheck.TCP_CHECK_SUCCESS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can AssertEquals
/AssertNotEqual
be used here and below instead of ==
/!=
?
a2da2a3
to
0a9be1f
Compare
got rid of importing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now. Thank you. We will clean this up even more over time.
This PR fails on GHA and also locally for me. I get
Can you look at that @andythsu ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One change, then this should be good to merge
Assert.assertFalse(checkObj == checker.getChecker(host, port, checkInterval + 10, | ||
assertEquals(checkObj, checker.getChecker(host, port, checkInterval + 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andythsu i believe the test failure was right here. The original was assertFalse(checkObj == checker...)
, so these should have been changed to assertNotEquals
. The intent of the test should be clearer this way :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh I see. Got tripped over the assertFalse
and ==
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch! Thanks @willmostly
cc4e5d1
to
1d79314
Compare
1d79314
to
796c8ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. All green and good to go now. Merging.
No description provided.