Add test coverage for disambiguation, related and similar entity scenarios#63
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThree test files expanded with comprehensive test coverage: TestGenesisService adds tests for similar/related entity scenarios and response format validation; TestDBpediaSparql refactored to standalone with 10 new SPARQL-focused tests; new TestDisambiguation class introduces 9 disambiguation handling tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive test coverage for disambiguation, related entities, and similar entity scenarios in the DBpedia chatbot. The changes enable validation of SPARQL query functionality for handling ambiguous entity names and entity relationships.
Changes:
- Added new test file TestDisambiguation.java with 9 test methods covering disambiguation page detection, entity retrieval, pagination, and edge cases
- Transformed TestDBpediaSparql.java from an ignored placeholder into an active test suite with 10 test methods for SPARQL query construction and entity resolution
- Extended TestGenesisService.java with 10 new test methods for similar/related entity retrieval across different entity types
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/test/java/rivescript/dbpedia/TestDisambiguation.java | New test suite validating disambiguation page detection, entity information retrieval, pagination support, and special character handling |
| src/test/java/rivescript/dbpedia/TestDBpediaSparql.java | Completely rewritten from ignored stub to comprehensive SPARQL query test suite covering disambiguation detection, batch retrieval, and query construction |
| src/test/java/chatbot/lib/api/dbpedia/TestGenesisService.java | Extended with tests for similar/related entity retrieval across politicians, countries, movies, and technologies, plus response format validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/test/java/chatbot/lib/api/dbpedia/TestGenesisService.java`:
- Around line 24-28: The test TestGenesisService.checkSimilar calls
GenesisService.getSimilarEntities and then uses assertNotEquals with the
arguments in the wrong order; switch the arguments so the unexpected value comes
first and the actual value second (e.g., assertNotEquals("", uris.trim())), or
replace with assertFalse(uris.trim().isEmpty()) — update the assertNotEquals
call in checkSimilar accordingly.
- Around line 34-39: In checkRelated(), the assertNotEquals call has arguments
reversed—swap them like in checkSimilar so the unexpected value is the first
argument; update the assertion in TestGenesisService.checkRelated to
assertNotEquals("", uris.trim()) (keeping the existing assertNotNull and using
GenesisService.getRelatedEntities as the source).
- Around line 115-126: The test checkDisambiguationVsSimilar (in
TestGenesisService) doesn't implement its Javadoc intent; it only calls new
GenesisService(0).getSimilarEntities("http://dbpedia.org/resource/Paris") and
asserts not-null. Update the test to either (A) rename it to
checkSimilarLocationEntity if you intend only to verify similar-entity
retrieval, or (B) enhance it to compare behavior between a disambiguation URI
and a specific entity: call GenesisService.getSimilarEntities on a known
disambiguation page URI and on "http://dbpedia.org/resource/Paris", then assert
that the disambiguation result differs (e.g., empty, contains disambiguation
markers, or lacks specific URIs) from the Paris result; keep the test method
name checkDisambiguationVsSimilar if you choose option B and use the existing
getSimilarEntities and GenesisService symbols to locate changes.
In `@src/test/java/rivescript/dbpedia/TestDisambiguation.java`:
- Around line 154-167: The test testEntityWithSpecialCharacters currently uses
Albert_Einstein which has no parentheses; update the test to use a DBpedia
resource that includes parentheses (e.g., "Python_(programming_language)" or
"Mercury_(planet)") by changing the uri passed to sparql.getEntityInformation
and adjust the method comment accordingly; keep assertions on ResponseData and
entity.getTitle() intact so sparql.getEntityInformation and ResponseData are
still validated for handling special characters.
🧹 Nitpick comments (6)
src/test/java/chatbot/lib/api/dbpedia/TestGenesisService.java (1)
45-49: Consider adding non-empty check for consistency with existing tests.The original
checkSimilarandcheckRelatedtests verify bothassertNotNullandassertNotEquals("", ...). The new tests only check for null. Consider adding the empty check if non-empty results are expected.src/test/java/rivescript/dbpedia/TestDisambiguation.java (3)
3-7: Remove unused imports.
ConstantsandCloudantClientare imported but never used in this test class.Proposed fix
package rivescript.dbpedia; -import chatbot.lib.Constants; import chatbot.lib.TestUtility; import chatbot.lib.api.SPARQL; import chatbot.lib.response.ResponseData; -import com.cloudant.client.api.CloudantClient; import org.junit.BeforeClass; import org.junit.Test;
23-30: Silent exception swallowing can mask configuration issues.When setup fails, the exception is caught and ignored. This causes all tests to silently "pass" (via early return) rather than being properly skipped. Consider using JUnit's
Assumeto mark tests as skipped when prerequisites are unavailable.Proposed improvement
+import static org.junit.Assume.assumeNotNull; + public class TestDisambiguation { private static SPARQL sparql; `@BeforeClass` public static void setUp() throws Exception { try { sparql = new SPARQL(TestUtility.getHelper().getExplorerDB()); } catch (Exception e) { - // If test utilities fail (missing credentials), tests will be skipped + System.err.println("SPARQL setup failed, tests will be skipped: " + e.getMessage()); } }Then in each test method, replace
if (sparql == null) return;with:assumeNotNull("SPARQL client not available", sparql);This properly marks tests as "skipped" rather than "passed".
131-137: Redundant assertion inside null-check.The
assertNotNull("Second page should exist", page2)at line 134 is inside a condition that already checkspage2 != null, so it will never fail.Proposed fix
assertNotNull("First page should exist", page1); // Verify different entities in pagination if (page2 != null && page1.size() > 0 && page2.size() > 0) { - assertNotNull("Second page should exist", page2); assertTrue("Pages should be different", !page1.get(0).getTitle().equals(page2.get(0).getTitle())); }src/test/java/rivescript/dbpedia/TestDBpediaSparql.java (2)
21-28: Same setup pattern issues as TestDisambiguation.java.This has the same silent exception swallowing issue. Consider using
Assume.assumeNotNull()to properly mark tests as skipped when SPARQL client is unavailable.
48-78: Significant test overlap with TestDisambiguation.java.Several tests in this file are nearly identical to tests in
TestDisambiguation.java:
testDisambiguationDetection≈testIsDisambiguationPagetestDisambiguatedEntityRetrieval≈testGetDisambiguatedEntitiestestDisambiguationPaginationappears in both filesConsider consolidating these into a shared test class or documenting why the duplication is intentional (e.g., testing from different entry points).
… and use appropriate assertion methods
GaneshPatil7517
force-pushed
the
test/disambiguation-related-similar
branch
from
6cafcb7 to
8b307cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@mvn/README.txt`:
- Line 36: The phrase "Unix based systems (Linux, Solaris and Mac OS X) and
others:" should use a hyphenated compound adjective; update the text to read
"Unix-based systems (Linux, Solaris and Mac OS X) and others:" by replacing
"Unix based" with "Unix-based" wherever that exact phrase appears.
In `@src/test/java/rivescript/dbpedia/TestDisambiguation.java`:
- Around line 13-15: Add the missing static import for assertNotEquals so the
TestDisambiguation class compiles: in the import block that currently includes
assertEquals, assertNotNull, and assertTrue, add the static import for
org.junit.Assert.assertNotEquals (or the equivalent testing framework's
assertNotEquals used elsewhere) so the call to assertNotEquals in
TestDisambiguation compiles successfully.
🧹 Nitpick comments (10)
mvn/lib/ext/README.txt (1)
1-2: Minor grammar fix: hyphenate "third-party" as a compound adjective.Per static analysis, "third party" should be hyphenated when used as a compound adjective modifying "extensions."
📝 Suggested fix
-Use this directory to add third party extensions to Maven Core. These extensions can either extend or override +Use this directory to add third-party extensions to Maven Core. These extensions can either extend or override Maven's default implementation.mvn/lib/plexus-interpolation.license (1)
1-202: Reconsider committing bundled Maven distribution files.This PR adds a full Maven distribution (licenses, configs, scripts) to the repository, but the PR objectives focus on adding test coverage for disambiguation scenarios. Bundling a Maven distribution is unusual—typically Maven is installed externally or managed via a wrapper script (
mvnw).Consider whether these files are intentionally included or were added by mistake. If Maven wrapper is needed, use
mvn -N wrapper:wrapperto generate a minimalmvnwsetup instead.src/test/java/rivescript/dbpedia/TestDisambiguation.java (2)
3-7: Remove unused imports.
Constants(line 3) andCloudantClient(line 7) are imported but never used in this test class.♻️ Proposed fix
package rivescript.dbpedia; -import chatbot.lib.Constants; import chatbot.lib.TestUtility; import chatbot.lib.api.SPARQL; import chatbot.lib.response.ResponseData; -import com.cloudant.client.api.CloudantClient; import org.junit.BeforeClass; import org.junit.Test;
23-30: Silent exception handling may hide real failures.The
catch (Exception e)block silently swallows all exceptions. Combined with theif (sparql == null) return;pattern in each test, this means tests will silently pass even if setup fails for unexpected reasons (not just missing credentials).Consider using JUnit's
Assumeto make test skipping explicit, or at least log the exception.♻️ Suggested approach using Assume
+import org.junit.Assume; + `@BeforeClass` public static void setUp() throws Exception { try { sparql = new SPARQL(TestUtility.getHelper().getExplorerDB()); } catch (Exception e) { - // If test utilities fail (missing credentials), tests will be skipped + System.err.println("SPARQL setup failed, tests will be skipped: " + e.getMessage()); } }Then in tests, replace
if (sparql == null) return;with:Assume.assumeNotNull("SPARQL not initialized - skipping test", sparql);This makes it clear in test reports that tests were skipped rather than passed.
src/test/java/rivescript/dbpedia/TestDBpediaSparql.java (3)
11-13: Consider addingassertNotEqualsimport for cleaner assertions.Line 146-147 uses
assertTrue(!firstPageFirstTitle.equals(secondPageFirstTitle)). UsingassertNotEqualswould be cleaner and provide better failure messages.♻️ Proposed fix
import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue;
142-148: UseassertNotEqualsinstead ofassertTruewith negation.The current pattern
assertTrue("...", !firstPageFirstTitle.equals(secondPageFirstTitle))is less readable and provides a less informative failure message thanassertNotEquals.♻️ Proposed fix
if (firstPage.size() > 0 && secondPage != null && secondPage.size() > 0) { // Verify pagination is working by comparing first entries String firstPageFirstTitle = firstPage.get(0).getTitle(); String secondPageFirstTitle = secondPage.get(0).getTitle(); - assertTrue("Pages should contain different entities", - !firstPageFirstTitle.equals(secondPageFirstTitle)); + assertNotEquals("Pages should contain different entities", + firstPageFirstTitle, secondPageFirstTitle); }
21-28: Same concern about silent exception handling.Same issue as in
TestDisambiguation.java- consider usingAssume.assumeNotNull(sparql)instead of theif (sparql == null) return;pattern to make test skipping explicit in reports.src/test/java/chatbot/lib/api/dbpedia/TestGenesisService.java (3)
41-89: Strengthen these tests to reject empty results.Right now they only assert non-null, so empty responses still pass. Consider using the same non-empty check as
checkSimilar/checkRelated.✅ Suggested assertions
public void checkSimilarDifferentType() throws Exception { String uris = new GenesisService(0).getSimilarEntities("http://dbpedia.org/resource/Germany"); assertNotNull("Similar entities should be retrievable for countries", uris); + assertNotEquals("", uris.trim()); } public void checkRelatedDifferentType() throws Exception { String uris = new GenesisService(0).getRelatedEntities("http://dbpedia.org/resource/Germany"); assertNotNull("Related entities should be retrievable for countries", uris); + assertNotEquals("", uris.trim()); } public void checkSimilarMovie() throws Exception { String uris = new GenesisService(0).getSimilarEntities("http://dbpedia.org/resource/The_Matrix"); assertNotNull("Similar movies should be retrievable", uris); + assertNotEquals("", uris.trim()); } public void checkRelatedMovie() throws Exception { String uris = new GenesisService(0).getRelatedEntities("http://dbpedia.org/resource/The_Matrix"); assertNotNull("Related movies should be retrievable", uris); + assertNotEquals("", uris.trim()); } public void checkSimilarTechnology() throws Exception { String uris = new GenesisService(0).getSimilarEntities("http://dbpedia.org/resource/Python_(programming_language)"); assertNotNull("Similar programming languages should be retrievable", uris); + assertNotEquals("", uris.trim()); }
115-125: Consider asserting non-empty results here as well.This currently allows empty responses to pass. Align with the non-empty checks used earlier.
✅ Suggested assertion
public void checkSimilarEntitiesForLocation() throws Exception { // Test with a specific location (not a disambiguation page) String similarCities = new GenesisService(0).getSimilarEntities("http://dbpedia.org/resource/Paris"); // Verify that similar cities are retrievable assertNotNull("Similar entities for specific location should be retrievable", similarCities); + assertNotEquals("", similarCities.trim()); }
91-113: Make URI format checks tolerant ofhttp/https.The backend returns URLs from the Genesis API which may use either protocol. Consider using a regex pattern to accept both:
- assertTrue("Response should contain DBpedia URIs in angle brackets", - uris.contains("<http://dbpedia.org/resource/")); + assertTrue("Response should contain DBpedia URIs in angle brackets", + uris.matches(".*<https?://dbpedia.org/resource/.*"));
…ns and enhance disambiguation comparison test
1 participant
This PR adds comprehensive test coverage for handling ambiguous entity names and entity relationships in the DBpedia chatbot. We're ensuring SPARQL queries correctly return results for disambiguation pages, related entities, and similar entities.
What's included
New test file: TestDisambiguation.java
Extended: TestGenesisService.java with 10 new test methods
Extended: TestDBpediaSparql.java with 10 new test methods
Coverage
Tests are defensive and follow existing conventions. Run with
mvn test.