Migrate Test Classes from TestNG to JUnit 5#74
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.
…-testng-with-junit
…junit' into BITMAG-1244-junit-bitrepository-core # Conflicts: # bitrepository-core/src/test/java/org/bitrepository/protocol/IntegrationTest.java # bitrepository-core/src/test/java/org/bitrepository/protocol/bus/ActiveMQMessageBusTest.java # bitrepository-core/src/test/java/org/bitrepository/protocol/performancetest/MessageBusNumberOfListenersStressTest.java
…n choose to include the test or not
…n install can't run because it gives errors in the module bitrepository-client, which hasn't been converted to junit 5 yet.
pom.xml
Outdated
| <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.
Please break this line in the middle and fix the indentation of lines 189 to 195.
pom.xml
Outdated
| <artifactId>testng</artifactId> | ||
| <version>7.1.0</version> | ||
| <groupId>org.mockito</groupId> | ||
| <artifactId>mockito-junit-jupiter</artifactId> |
There was a problem hiding this comment.
Do we need this dependency? What for?
There was a problem hiding this comment.
You're correct. Removed it.
bitrepository-alarm-service/src/test/java/org/bitrepository/alarm/handler/AlarmHandlerTest.java
Outdated
Show resolved
Hide resolved
| Assertions.assertNull(model.getFileID()); | ||
| Assertions.assertNull(model.getStartDate()); | ||
| Assertions.assertNull(model.getCollectionID()); | ||
| Assertions.assertEquals(model.getAscending(), ascending); |
There was a problem hiding this comment.
You have mostly remembered to swap arguments of assertEquals() and assertNotEquals(). Seems you forgot in some places throughout this pull request, like here and few more times in this file. IntelliJ IDEA correctly warns about it most of the times (at least mine does, I believe it’s a default setting). We have agreed that this can be fixed in a different pull request.
There was a problem hiding this comment.
Found out that there it is possible to change everywhere that there is a warning in the project, so it should be done now.
bitrepository-alarm-service/src/test/java/org/bitrepository/alarm/store/AlarmDatabaseTest.java
Show resolved
Hide resolved
...y-audit-trail-service/src/test/java/org/bitrepository/audittrails/AuditTrailServiceTest.java
Show resolved
Hide resolved
| return getClass().getSimpleName(); | ||
| } | ||
|
|
||
| protected String createDate() { |
There was a problem hiding this comment.
The method does not seem to be used ever. Delete?
| protected void shutdownCUT() {} | ||
|
|
||
| /** | ||
| * Initializes the settings. Will postfix the alarm and collection topics with '-${user.name} |
There was a problem hiding this comment.
Nitpicking, should the single quote be closed after the right curly brace?
There was a problem hiding this comment.
If this file or parts of it are copy-pasted from somewhere and/or AI generated, please give the source in a comment.
| protected static Settings settingsForCUT; | ||
| protected static Settings settingsForTestClient; | ||
| protected static String collectionID; | ||
| protected String NON_DEFAULT_FILE_ID; |
There was a problem hiding this comment.
Uppercase (screaming snake case) is reserved for constants, that is, variables that are static and final. So please either make the variable so or change the name. Same for other names in all uppercase.
| /** | ||
| * May be overridden by specific tests wishing to do stuff. Remember to call super if this is overridden. | ||
| */ | ||
| protected void shutdownCUT() {} |
There was a problem hiding this comment.
Should this be called, for example from afterAll()?
2 participants
Description:
This pull request migrates all test classes from TestNG to JUnit 5. The migration includes replacing TestNG-specific annotations and methods with their JUnit 5 equivalents. The migration of JAccept is still pending, and classes where JUnit 5 does not have a similar method should be reviewed more thoroughly.
Changes:
Migration of TestNG to JUnit 5:
Replaced TestNG annotations (@BeforeClass, @test, @tag, etc.) with JUnit 5 equivalents (@BeforeAll, @test, @tag, etc.).
Updated test lifecycle methods to use JUnit 5 annotations (@beforeeach, @AfterEach, @BeforeAll, @afterall).
Review of JUnit 5 Compatibility:
It will be appreciated if classes where JUnit 5 does not have a direct equivalent method is reviewed thoroughly. These classes have been updated to use appropriate JUnit 5 methods.
Pending JAccept Migration:
The migration of JAccept is still pending. The test classes have been updated to use JUnit 5 annotations and methods, but JAccept-specific methods and classes have not been replaced.
Removal of IntegrityWorkflowSchedulerTest.java:
Observed that IntegrityWorkflowSchedulerTest.java might be redundant and has been marked for removal. This class will be reviewed and removed if deemed unnecessary.
Detailed Changes:
Updated Annotations:
Replaced @BeforeClass with @BeforeAll.
Replaced @test with @test.
Replaced @groups with @tag.
Lifecycle Methods:
Updated test lifecycle methods to use JUnit 5 annotations (@beforeeach, @AfterEach, @BeforeAll, @afterall).
Pending JAccept Migration:
The migration of JAccept is still pending. The test classes have been updated to use JUnit 5 annotations and methods, but JAccept-specific methods and classes have not been replaced.
Removal of IntegrityWorkflowSchedulerTest.java:
Shouldn't IntegrityWorkflowSchedulerTest.java be removed from the project?. This class will be reviewed and removed if deemed unnecessary.
Review Request:
Please review the changes thoroughly. Specifically, pay attention to the classes where JUnit 5 does not have a direct equivalent method and ensure that the migration are correct and functional. In the master branch there are tests that fail, so the same tests probably will fail after migration. Migration of JAccept that will follow may also inflict on tests that fail, so the migration of the tests should at most amount in the same tests as in the master branch will fail. Also, review the removal of IntegrityWorkflowSchedulerTest.java and provide feedback on whether this class should be removed from the project.