chore: refactor assertions to AssertJ

[robfrank]

What does this PR do?

Refactors tests to use AssertJ assertions

[mergify]

🧪 CI Insights

Here's what we observed from your CI run for 7ea85f8.

🟢 All jobs passed!

But CI Insights is watching 👀

[claude]

Code Review: AssertJ Migration

Overview

This PR migrates test assertions from JUnit's classic assertion methods to AssertJ's fluent assertion API across 178 test files. This is a large-scale refactoring focused on improving test readability and maintainability.

✅ Strengths

1. Consistent Migration Pattern: The refactoring follows a consistent pattern throughout:

   • assertEquals() → assertThat().isEqualTo()
   • assertTrue() → assertThat().isTrue()
   • assertFalse() → assertThat().isFalse()
   • assertNotNull() → assertThat().isNotNull()
   • assertNull() → assertThat().isNull()

2. Improved Readability: AssertJ's fluent API makes tests more readable, especially for complex assertions:

   • Before: assertFalse(results.isEmpty(), "Should find similar documents")
   • After: assertThat(results).isNotEmpty()

3. Better Exception Testing: The migration to assertThatThrownBy() is cleaner than the old assertThrows() pattern and eliminates try-catch blocks with Assertions.fail().

4. Test Method Naming: Removed test prefix from many test method names (e.g., testConnection() → connection()), which is a modern JUnit 5 best practice since the @Test annotation already identifies test methods.

5. Fluent Chaining: Good use of method chaining for cleaner assertions:

   • assertThat(result).isNotNull().isNotEmpty()

⚠️ Issues & Concerns

1. Sub-optimal Assertion Patterns

Several assertions could be more idiomatic:

Issue: Using .contains().isTrue() instead of direct collection assertions

// Current (lines like in MoreLikeThisQueryBuilderTest.java)
assertThat(terms.contains("java")).isTrue();
assertThat(terms.contains("database")).isTrue();

// Better
assertThat(terms).contains("java", "database");

Issue: Using .isEmpty().isTrue() instead of direct isEmpty assertion

// Current
assertThat(terms).isNotNull();
assertThat(terms.isEmpty()).isTrue();

// Better
assertThat(terms).isEmpty();

Issue: Complex boolean expressions wrapped in assertThat

// Current (OpenCypherReduceAndShortestPathTest.java:266)
assertThat(path == null || (path instanceof List && ((List<?>) path).isEmpty())).isTrue();

// Better - split into separate assertions or use satisfies()
assertThat(path).satisfiesAnyOf(
    p -> assertThat(p).isNull(),
    p -> assertThat((List<?>) p).isEmpty()
);

Issue: Using size() > 0 instead of isNotEmpty()

// Current (LSMVectorIndexGraphStorageTest.java)
assertThat(results.size() > 0).as("Should find neighbors").isTrue();

// Better
assertThat(results).isNotEmpty().as("Should find neighbors");

2. Missing Assertion Messages

Many assertions lost their descriptive messages during migration:

// Before
assertFalse(results.isEmpty(), "Should find similar documents");
assertTrue(results.contains(javaDatabaseRID), "Should find Java Database document (similar)");

// After - messages lost
assertThat(results).isNotEmpty();
assertThat(results).contains(javaDatabaseRID);

While AssertJ provides good default messages, custom messages are valuable for domain-specific context, especially for complex business logic tests.

3. Inconsistent Patterns

Some files show inconsistent assertion styles:

// Mixed approaches in same file
assertThat(terms.contains("java")).isTrue();  // Using boolean wrapper
assertThat(results).contains(javaDatabaseRID);  // Direct collection assertion

4. Size Assertions Could Be More Specific

// Current
assertThat(terms.size()).isEqualTo(2);

// Better - more fluent
assertThat(terms).hasSize(2);

📋 Recommendations

High Priority

1. Refine collection assertions to use AssertJ's collection-specific methods:

   • Replace assertThat(collection.contains(x)).isTrue() with assertThat(collection).contains(x)
   • Replace assertThat(collection.isEmpty()).isTrue() with assertThat(collection).isEmpty()
   • Replace assertThat(collection.size()).isEqualTo(n) with assertThat(collection).hasSize(n)

2. Simplify boolean expression assertions to avoid assertThat(booleanExpr).isTrue() where better alternatives exist.

3. Consider restoring assertion messages for tests with complex business logic where the default AssertJ message might not provide sufficient context.

Medium Priority

4. Verify test coverage: Run the full test suite to ensure no behavioral changes were introduced. The refactoring should be purely mechanical.

5. Check for floating-point comparisons: Ensure any assertEquals with delta parameters were properly migrated to use isCloseTo() with within().

Low Priority

6. Add as() descriptions to complex assertions to improve failure messages, especially in LSM vector index tests where numerical comparisons are involved.

🔍 Security & Performance

• ✅ No security concerns - purely test refactoring
• ✅ No performance impact on production code
• ✅ Test execution performance should be identical

📊 Test Coverage

The PR description states this is a refactoring, but it would be valuable to:

• ✅ Ensure all tests still pass (CI should verify)
• ⚠️ Manually spot-check a few complex test files to ensure assertion semantics are preserved

Verdict

Status: ⚠️ Approved with Suggestions

This is a valuable refactoring that modernizes the test suite and improves readability. The migration is mostly correct and follows good practices. However, there are opportunities to make the assertions more idiomatic by using AssertJ's specialized collection and size assertions instead of wrapping boolean expressions.

Suggested Next Steps:

1. Optional follow-up PR: Refine the assertions to use more idiomatic AssertJ patterns (collection-specific assertions)
2. Verify all tests pass: Ensure CI completes successfully
3. Spot-check critical tests: Manually verify a few complex tests (e.g., vector index tests, cypher function tests) to ensure semantics are preserved

The current state is acceptable for merging since it's functionally equivalent and a clear improvement over the old style, but the refinements would make it even better.

---

Alignment with CLAUDE.md: ✅

• Follows existing test patterns
• No new dependencies introduced
• Adheres to project's preference for assertThat() syntax mentioned in CLAUDE.md
• Maintains test coverage requirements

[gemini-code-assist]

Summary of Changes

Hello @robfrank, I'm Gemini Code Assist¹! 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 significantly enhances the readability and maintainability of the test suite by adopting AssertJ's fluent assertion style. This modernization effort standardizes assertion practices, making tests easier to understand and write, and aligns the project with contemporary Java testing best practices.

Highlights

• Test Assertion Refactoring: The primary change involves migrating test assertions from JUnit's built-in methods (e.g., assertTrue, assertEquals) to the more expressive and fluent AssertJ library's assertThat API across numerous test files.
• Test Method Renaming: Many test methods have been renamed by removing the test prefix (e.g., testCreateVertexWithMultipleLabels is now createVertexWithMultipleLabels) to align with modern JUnit 5 conventions.
• Improved Exception Assertions: JUnit's assertThrows and manual try-catch blocks for exception testing have been replaced with AssertJ's assertThatThrownBy for cleaner and more readable exception assertions.
• Test Class Visibility Adjustments: Several test classes have had their visibility changed from public to package-private, which is a common practice for unit tests that are not intended for external consumption.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request is a large refactoring of the test suite to standardize on AssertJ for assertions and to adopt a new naming convention for test methods. The changes are generally positive, improving consistency and readability. However, I've noticed a recurring pattern of replacing specific checked exceptions in throws clauses with the generic Exception. This reduces the clarity of the method signatures, and I've left comments suggesting to revert these changes to be more specific about the exceptions thrown.