Migrate tests from TestNG to JUnit 4 in tests module #620
Conversation
Summary of ChangesHello @andrii0lomakin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant refactoring of the test suite by transitioning from TestNG to JUnit 4. The primary goal is to improve the developer experience by leveraging JUnit 4's widespread IDE integration and simplified configuration, thereby reducing friction in test creation and execution. The changes span across numerous test files and build configurations to achieve this modernization. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request represents a significant and valuable migration of the test suite from TestNG to JUnit 4. The changes are extensive, touching the build configuration, adding new base test classes, and converting a large number of test files. This effort will certainly simplify the development and testing workflow. My review focuses on ensuring the new configuration is correct, the migrated tests are robust, and that best practices are followed in the new test infrastructure. I've identified a few areas for improvement, including a critical issue with Java version compatibility, some potential bugs in test logic, and opportunities to improve performance and maintainability in the new test base classes.
...rc/test/java/com/jetbrains/youtrackdb/auto/junit/databasesuite/CRUDDocumentPhysicalTest.java
Show resolved
Hide resolved
| for (var id : ids) { | ||
| session.begin(); | ||
| var element = session.newEntity("Account"); | ||
| element.setProperty("id", id); | ||
| element.setProperty("name", "Gipsy"); | ||
| element.setProperty("location", "Italy"); | ||
| element.setProperty("testLong", 10000000000L); | ||
| element.setProperty("salary", id + 300); | ||
| element.setProperty("extra", "This is an extra field not included in the schema"); | ||
| element.setProperty("value", (byte) 10); | ||
|
|
||
| binary = new byte[100]; | ||
| for (var b = 0; b < binary.length; ++b) { | ||
| binary[b] = (byte) b; | ||
| } | ||
| element.setProperty("binary", binary); | ||
| Assert.assertTrue(accountCollectionIds.contains(element.getIdentity().getCollectionId())); | ||
|
|
||
| session.commit(); | ||
| } |
There was a problem hiding this comment.
The fillInAccountData method performs a session.begin() and session.commit() for each record being inserted inside the loop. This is highly inefficient as it creates a new transaction for every single record. For better performance, the transaction should be started once before the loop and committed once after the loop has completed.
session.begin();
for (var id : ids) {
var element = session.newEntity("Account");
element.setProperty("id", id);
element.setProperty("name", "Gipsy");
element.setProperty("location", "Italy");
element.setProperty("testLong", 10000000000L);
element.setProperty("salary", id + 300);
element.setProperty("extra", "This is an extra field not included in the schema");
element.setProperty("value", (byte) 10);
binary = new byte[100];
for (var b = 0; b < binary.length; ++b) {
binary[b] = (byte) b;
}
element.setProperty("binary", binary);
Assert.assertTrue(accountCollectionIds.contains(element.getIdentity().getCollectionId()));
}
session.commit();| <youtrackdb.storage.makeFullCheckpointAfterCreate>false | ||
| </youtrackdb.storage.makeFullCheckpointAfterCreate> | ||
| <storage.makeFullCheckpointAfterCollectionCreate>false | ||
| </storage.makeFullCheckpointAfterCollectionCreate> | ||
| <storage.wal.syncOnPageFlush>false</storage.wal.syncOnPageFlush> | ||
| <storage.configuration.syncOnUpdate>false</storage.configuration.syncOnUpdate> | ||
| <index.flushAfterCreate>false</index.flushAfterCreate> | ||
| <youtrackdb.security.userPasswordSaltIterations>10 | ||
| </youtrackdb.security.userPasswordSaltIterations> | ||
| <buildDirectory>${project.build.directory}</buildDirectory> | ||
| <testPath>${project.basedir}</testPath> | ||
| <youtrackdb.memory.directMemory.preallocate>false | ||
| </youtrackdb.memory.directMemory.preallocate> |
There was a problem hiding this comment.
The values for several system properties are split across multiple lines. This causes the property value to include leading whitespace and newline characters from the XML formatting, which can lead to unexpected behavior or parsing errors. For example, the value for youtrackdb.storage.makeFullCheckpointAfterCreate will be false\n instead of just false. All property values should be on a single line within their tags.
| <youtrackdb.storage.makeFullCheckpointAfterCreate>false | |
| </youtrackdb.storage.makeFullCheckpointAfterCreate> | |
| <storage.makeFullCheckpointAfterCollectionCreate>false | |
| </storage.makeFullCheckpointAfterCollectionCreate> | |
| <storage.wal.syncOnPageFlush>false</storage.wal.syncOnPageFlush> | |
| <storage.configuration.syncOnUpdate>false</storage.configuration.syncOnUpdate> | |
| <index.flushAfterCreate>false</index.flushAfterCreate> | |
| <youtrackdb.security.userPasswordSaltIterations>10 | |
| </youtrackdb.security.userPasswordSaltIterations> | |
| <buildDirectory>${project.build.directory}</buildDirectory> | |
| <testPath>${project.basedir}</testPath> | |
| <youtrackdb.memory.directMemory.preallocate>false | |
| </youtrackdb.memory.directMemory.preallocate> | |
| <youtrackdb.storage.makeFullCheckpointAfterCreate>false</youtrackdb.storage.makeFullCheckpointAfterCreate> | |
| <storage.makeFullCheckpointAfterCollectionCreate>false</storage.makeFullCheckpointAfterCollectionCreate> | |
| <storage.wal.syncOnPageFlush>false</storage.wal.syncOnPageFlush> | |
| <storage.configuration.syncOnUpdate>false</storage.configuration.syncOnUpdate> | |
| <index.flushAfterCreate>false</index.flushAfterCreate> | |
| <youtrackdb.security.userPasswordSaltIterations>10</youtrackdb.security.userPasswordSaltIterations> | |
| <buildDirectory>${project.build.directory}</buildDirectory> | |
| <testPath>${project.basedir}</testPath> | |
| <youtrackdb.memory.directMemory.preallocate>false</youtrackdb.memory.directMemory.preallocate> |
| try { | ||
| session.begin(); | ||
| session.newEntity("AbstractPerson"); | ||
| session.begin(); |
There was a problem hiding this comment.
The test test01_CannotCreateInstances is designed to verify that creating an instance of an abstract class throws an exception. However, if session.newEntity("AbstractPerson") fails to throw the expected exception, the test will proceed to the next line (session.begin()) and then complete successfully without any assertion failures. This can mask a potential bug. You should add Assert.fail() after the call that is expected to throw to ensure the test fails if the exception is not thrown.
session.newEntity("AbstractPerson");
Assert.fail("Expected SchemaException to be thrown.");| protected void assertEmbedded(boolean isEmbedded) { | ||
| Assert.assertTrue((!isEmbedded)); | ||
| } |
There was a problem hiding this comment.
The method name assertEmbedded is misleading. It suggests that it asserts that the link bag is embedded, but the implementation Assert.assertTrue((!isEmbedded)) actually asserts the opposite. This can cause confusion for anyone reading or maintaining the test. Consider renaming the method to assertNotEmbedded to accurately reflect its behavior.
| protected void assertEmbedded(boolean isEmbedded) { | |
| Assert.assertTrue((!isEmbedded)); | |
| } | |
| protected void assertNotEmbedded(boolean isEmbedded) { | |
| Assert.assertFalse(isEmbedded); | |
| } |
| var activeTx = session.getActiveTransaction(); | ||
| var account = activeTx.<EntityImpl>load(accountRid); | ||
| record.setProperty("account", account); | ||
| record.setPropertyInChain("date", new SimpleDateFormat("dd/MM/yyyy").parse("01/33/1976")); |
There was a problem hiding this comment.
The SimpleDateFormat class is lenient by default and may parse an invalid date like 01/33/1976 into a valid one by rolling over the date (e.g., into February). This can make the test pass even if the input is not what you intend to validate against. To make this test more robust and explicit, consider either using a date that is unambiguously invalid or setting setLenient(false) on the SimpleDateFormat instance to ensure it throws a ParseException for invalid dates.
Enable and fix JUnit tests that were incorrectly marked as @ignore during migration from TestNG. Key fixes include: - BinaryTest: Make rid field static to preserve state across test methods - ConcurrentQueriesTest: Call init() in setUpClass to create test class - ConcurrentUpdatesTest: Fix assertEquals argument order (JUnit uses expected, actual vs TestNG actual, expected) - GraphDatabaseTest: Add test00_Populate to generate graph data, create "owns" edge class for test01_SQLAgainstGraph - IndexManagerTest: Add missing composite index creation tests (test01a_CreateCompositeIndexTestWithoutListener and test01b_CreateCompositeIndexTestWithListener), fix assertEquals order - HookOnIndexedMapTest: Change to expect IllegalStateException since the BrokenMapHook pattern correctly triggers this error All 838 tests now pass. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Create LinkBagTest abstract base class with 28 test methods - Create EmbeddedLinkBagTest extending LinkBagTest with embedded mode assertions - Add EmbeddedLinkBagTest to DatabaseTestSuite - Add JUnit execution configuration (test-junit) to pom.xml - Include comments with references to original TestNG test locations Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add skip=true to test-embedded execution to disable TestNG tests - Remove surefire-testng plugin dependency to prevent TestNG provider activation Co-Authored-By: Claude Opus 4.5 <[email protected]>
Configure the test-junit Maven execution with necessary system properties for storage, security, and path settings. Also fix HookOnIndexedMapTest to use current directory for database path since in-memory databases don't create directories, causing "directory not exists" errors with the original path. Co-Authored-By: Claude Opus 4.5 <[email protected]>
The JUnit version was missing the @before and @after methods that configure the link collection thresholds to -1, which forces BTree mode instead of embedded mode for link bags during tests. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- LinkBagTest.test14_Remove(): Add missing session.begin() and commit() - LinkBagTest.test24_RemoveNotExistentElementAndAddIt(): Add session.begin() after first commit and commit() at end - LinkBagTest.test25_AddNewItemsAndRemoveThem(): Add session.commit() before nested session.begin() - CRUDTest.test32_TestEmbeddedBinary(): Add missing session.begin() - TransactionAtomicTest.test02_MVCC(): Add session.rollback() in catch block - SQLSelectTest.beforeClass(): Add missing super.beforeClass() call Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add @BeforeClass and @after cleanup methods to TransactionConsistencyTest for proper database session lifecycle management - Replace silent test skips (if/return) with Assume.assumeFalse() in BTreeBasedLinkBagTest for better test visibility - Fix assertEquals argument order (expected, actual) in BTreeBasedLinkBagTest and LinkBagTest - Replace Assert.assertTrue((!x)) with Assert.assertFalse(x) for clarity - Remove dead code (self-assignments) in TransactionConsistencyTest - Fix resource leak where database2 was reassigned without closing Co-Authored-By: Claude Opus 4.5 <[email protected]>
…Unit tests - Add @BeforeClass to 6 test classes that were missing it: TransactionAtomicTest, FrontendTransactionImplTest, ComplexTypesTest, DBRecordCreateTest, DBRecordMetadataTest, and CRUDDocumentValidationTest - Fix thread safety issue in SchemaTest by adding session.activateOnCurrentThread() - Add comprehensive suite dependency documentation to DatabaseTestSuite explaining test groups, execution order, and guidelines for adding new tests - Document suite dependency in BaseTest, BaseDBTest, and individual test classes including abstract base classes (LinkBagTest, AbstractIndexReuseTest, IndexTxAwareBaseTest) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Reorganize JUnit test classes to clearly indicate their membership in DatabaseTestSuite by moving them to a dedicated subpackage. Changes: - Move 90 test classes to com.jetbrains.youtrackdb.auto.junit.databasesuite - Keep base/support classes in parent package (BaseTest, BaseDBTest, IndexTxAwareBaseTest, AbstractIndexReuseTest, LinkBagTest, DatabaseTestSuite) - Update package declarations and imports in all moved files - Update DatabaseTestSuite imports to reference new subpackage - Update javadoc @see references in base classes Co-Authored-By: Claude Opus 4.5 <[email protected]>
Qodana for JVM2040 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at [email protected]
|
andrii0lomakin
force-pushed
the
junitj-migration
branch
from
2e705c7 to
904f3bc
Compare
Motivation:
TestNG requires XML configuration files and specialized IDE plugins to run tests effectively, which complicates the development workflow. JUnit 4 offers better out-of-the-box support in all major IDEs (IntelliJ IDEA, Eclipse, VS Code) and CLI tools (Maven Surefire, Gradle), enabling developers to run individual tests or test classes directly without additional configuration. This migration simplifies the developer experience and reduces friction when working with the test suite.
Changes:
com.jetbrains.youtrackdb.auto.junitBaseTest.java- Foundation class handling database setup/teardown with@Before/@AfterlifecycleBaseDBTest.java- Extended base class for database-specific testsDatabaseTestSuite.java- JUnit test suite aggregating all migrated teststests/pom.xml:<skip>true</skip>)test-junitexecution phase withsurefire-junit47provider@Test(TestNG) →@Test(JUnit 4)@BeforeClass/@AfterClass→@BeforeClass/@AfterClass(withstaticmethods)@BeforeMethod/@AfterMethod→@Before/@After@Parameters(TestNG) → Constructor-based or method-based parameterization