-
Notifications
You must be signed in to change notification settings - Fork 4
Initial migration from TestNG to JUnit 5 #73
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
base: master
Are you sure you want to change the base?
Conversation
- Added JUnit 5 annotations to BitrepositoryTestSuite - Added GlobalSuiteExtension to implement JUnit 5 extension points for suite-level setup and teardown. - Ensured consistency and correctness of test suite configuration and setup methods. This commit updates the test suite configuration to use JUnit 5 annotations, allowing for more flexible and powerful test suite management.
| import org.bitrepository.protocol.security.DummySecurityManager; | ||
| import org.bitrepository.protocol.security.SecurityManager; | ||
| import org.bitrepository.settings.repositorysettings.MessageBusConfiguration; | ||
| import org.jaccept.structure.ExtendedTestCase; |
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.
JAccept relies on TestNG. I got no idea whether we can get rid of both at the same time, or how great of an effort it would require. Ideally I think we would want to.
| import org.testng.Assert; | ||
| import org.testng.annotations.BeforeMethod; | ||
| import org.testng.annotations.Test; | ||
| //import org.testng.Assert; |
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.
Please delete rather than comment out.
| */ | ||
| @Test(groups = {"StressTest"}) | ||
| @Test | ||
| @Tag("StressTest") |
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, the way I read it this exactly the right substitute for TestNG groups.
| DEFAULT_DOWNLOAD_FILE_ADDRESS = DEFAULT_FILE_URL.toExternalForm(); | ||
| DEFAULT_UPLOAD_FILE_ADDRESS = DEFAULT_FILE_URL.toExternalForm() + "-" + DEFAULT_FILE_ID; | ||
| } catch (MalformedURLException e) { | ||
| throw new RuntimeException("Never happens"); |
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.
Allow me to suggest:
fail("Never happens", e);
I believe this will cause the test failure (although it never happens) to be clearer. At least somehow include the exception or its message in whatever we throw. If you like the RuntimeException better, you may chain the exception we’ve just caught:
throw new RuntimeException("Never happens", e);
(I just discovered you copied this code from the existing IntegrationTest class, so you may argue that improving the code is not part of this task.)
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.
I added the exception to RuntimeException.
| * </pre> | ||
| */ | ||
| @Suite | ||
| @SelectClasses({IntegrationTest.class}) // List your test classes here |
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.
Are we sure what the effect of including an abstract TestNG test class as a class in a JUnit test suite is?
| * public class BitrepositoryTestSuite { | ||
| * // No need for methods here; this just groups and extends | ||
| * } | ||
| * } |
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.
Does doubling this line do something good?
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.
Removed them and the public class BitrepositoryTestSutie above
| * May be extended by subclasses needing to have their receivers managed. Remember to still call | ||
| * <code>super.registerReceivers()</code> when overriding | ||
| */ | ||
| protected void registerMessageReceivers() { |
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 method is not used?
In the IntegrationTest class where you copied it from, it is called from beforeMethod(). Should this copy be called from somewhere too?
There are more unused methods in this class.
| // | ||
| } | ||
|
|
||
| private void initializationMethod() { |
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.
If I read it correctly, you have moved the call to this method from "before suite" to "before class". Is this correct? Is it necessary?
| messageBus.sendMessage(messageToSend); | ||
| Message receivedMessage = messageList.poll(3, TimeUnit.SECONDS); | ||
| assertEquals(receivedMessage.getStringProperty(ActiveMQMessageBus.MESSAGE_TO_KEY), receiverID); | ||
| Assertions.assertEquals(receivedMessage.getStringProperty(ActiveMQMessageBus.MESSAGE_TO_KEY), receiverID); // Assertion update |
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.
If I am not mistaken, TestNG and JUnit have the arguments to assertEquals() in opposite orders. JUnit: assertEquals(expected, actual). TestNG: assertEquals(actual, expected). If so, you need to swap the arguments when exchanging one for the other. My suggestion:
Assertions.assertNotNull(receivedMessage);
Assertions.assertEquals(receiverID, receivedMessage.getStringProperty(ActiveMQMessageBus.MESSAGE_TO_KEY));
The same argument swap applies everywhere, of course.
poll() may return null, so I added a check for that.
(I didn’t see the long-term value of the comment, so I left it out, but I may be missing something?)
| @Test(groups = {"regressiontest"}) | ||
| @Test | ||
| @Tag("regressiontest") | ||
| public final void collectionFilterTest() throws Exception { |
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.
I am sorry, this is not working. When I do mvn package, this test is not being run. And when I run this test class from within IntelliJ IDEA, the tests fail.
| @Test(groups = {"StressTest"}) | ||
| @Test | ||
| @Tag("StressTest") | ||
| public void testManyListenersOnLocalMessageBus() throws Exception { |
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.
I am sorry, this is not working. When I do mvn package, this test is not being run. And when I run this test class from within IntelliJ IDEA, the tests fail.
| <artifactId>junit-platform-suite-engine</artifactId> | ||
| <version>1.10.3</version> <!-- Match your JUnit version --> | ||
| <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.
Please fix the formatting and indentation of dependencies.
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.
Done
This commit updates the test suite configuration to use JUnit 5 annotations, allowing for more flexible and powerful test suite management.
It doesn't handle the TestNG @dependsOnGroups, because it collides with one of the principles of JUnit, which is that the test should run without depending on other tests.