add unit tests - #27
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive unit tests to the mongocom library, replacing the outdated JUnit 4 configuration with modern JUnit 5 and Mockito for better testing capabilities.
- Adds comprehensive unit test coverage for core library components
- Migrates from JUnit 4 to JUnit 5 with Mockito integration
- Fixes documentation formatting issues in JavaDoc comments
Reviewed Changes
Copilot reviewed 20 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Multiple test files | New unit tests covering utilities, types, management classes, exceptions, and annotations |
| pom.xml | Updated to JUnit 5 and added Mockito dependencies with Surefire plugin configuration |
| Type classes | Fixed HTML encoding in JavaDoc author tags |
| CollectionManager.java | Minor code improvements and documentation fixes |
| MongoQuery.java | Return defensive copies and improved documentation |
| CollectionManagerFactory.java | Enhanced exception handling for illegal arguments |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| public Document getQuery() { | ||
| return query; | ||
| return query != null ? new Document(query) : null; | ||
| } | ||
|
|
||
| public Document getConstraints() { | ||
| return constraints; | ||
| return constraints != null ? new Document(constraints) : null; | ||
| } | ||
|
|
||
| public Document getOrderBy() { |
There was a problem hiding this comment.
[nitpick] The defensive copying approach is inconsistent with the original implementation and may break existing code that expects direct references. Consider documenting this breaking change or providing configuration options to maintain backward compatibility.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #27 +/- ##
=============================================
+ Coverage 0.00% 90.46% +90.46%
- Complexity 0 182 +182
=============================================
Files 10 10
Lines 537 556 +19
Branches 97 100 +3
=============================================
+ Hits 0 503 +503
+ Misses 537 38 -499
- Partials 0 15 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
57bff32 to
021c1bd
Compare
021c1bd to
7570a48
Compare
|
|
/summary |
🤖 ThrillhouseBot PR SummaryWhat this PR doesPrimarily adds a large suite of unit tests covering annotations, exceptions, CollectionManager, and utilities using JUnit 5 and Mockito. Beyond tests, the code refactors CollectionManager to use getDeclaredConstructor().newInstance() instead of deprecated Class.newInstance(), renames the public method createCollectionManagerFromURI to createCollectionManagerFromUri, adds thread-safety when creating MongoClient instances, and implements defensive copies in MongoQuery getters.
|
| File | Change | Summary |
|---|---|---|
.copilot-instructions.md |
Modified | - |
.github/workflows/ci.yml |
Modified | - |
.github/workflows/pr-validation.yml |
Modified | - |
README.md |
Modified | - |
examples/src/com/example/collections/Address.java |
Modified | - |
examples/src/com/example/collections/Contact.java |
Modified | - |
examples/src/com/example/collections/Phone.java |
Modified | - |
examples/src/com/example/collections/types/ContactType.java |
Modified | - |
examples/src/com/example/collections/types/PhoneType.java |
Modified | - |
examples/src/com/example/main/Main.java |
Modified | - |
src/main/java/com/arquivolivre/mongocom/annotations/Document.java |
Modified | - |
src/main/java/com/arquivolivre/mongocom/annotations/GeneratedValue.java |
Modified | - |
src/main/java/com/arquivolivre/mongocom/annotations/Id.java |
Modified | - |
src/main/java/com/arquivolivre/mongocom/annotations/Index.java |
Modified | - |
src/main/java/com/arquivolivre/mongocom/annotations/Internal.java |
Modified | - |
src/main/java/com/arquivolivre/mongocom/annotations/ObjectId.java |
Modified | - |
src/main/java/com/arquivolivre/mongocom/annotations/Reference.java |
Modified | - |
src/main/java/com/arquivolivre/mongocom/annotations/Trigger.java |
Modified | - |
src/main/java/com/arquivolivre/mongocom/exceptions/NoSuchMongoCollectionException.java |
Modified | - |
src/main/java/com/arquivolivre/mongocom/management/CollectionManager.java |
Modified | Major changes: replace deprecated reflection APIs, add ClassCastException handling for _id, wrap setAccessible with AccessController, import reorganization, and Javadoc additions. |
…and 22 more file(s).
Risk Assessment
| Risk | Count |
|---|---|
| 🔴 Critical | 0 |
| 🟠 High | 1 |
| 🟡 Medium | 1 |
| 🔵 Low | 1 |
Key Findings
- HIGH: Breaking API rename from createCollectionManagerFromURI to createCollectionManagerFromUri (
src/main/java/com/arquivolivre/mongocom/management/CollectionManagerFactory.java:106) - MEDIUM: Client closure in setup/getConn may break existing CollectionManager instances (
src/main/java/com/arquivolivre/mongocom/management/CollectionManagerFactory.java:225) - LOW: Description gap: PR only mentions adding tests but includes functional changes (
src/main/java/com/arquivolivre/mongocom/management/CollectionManagerFactory.java:176)
⚠️ Required CI Checks Status
Some required checks are still pending or have failed:
| Check | Type | Status | Detail |
|---|---|---|---|
| SonarCloud Code Analysis | check-run | ❌ Failed | failure |
Automated review by ThrillhouseBot. Reply with /review to re-run.
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
| @@ -73,21 +106,14 @@ public static CollectionManager createCollectionManager( | |||
| * @param uri MongoDB connection URI (e.g., "mongodb://user:password@host:port/database") | |||
There was a problem hiding this comment.
🟠 HIGH — Breaking API rename from createCollectionManagerFromURI to createCollectionManagerFromUri
The method name changed from createCollectionManagerFromURI to createCollectionManagerFromUri, altering the public API. This will cause compilation failures in any code that calls the old name. The PR description only mentions adding unit tests and does not warn of this breaking change. Change: line 106 shows the new signature public static CollectionManager createCollectionManagerFromUri(String uri).
| * @param uri MongoDB connection URI (e.g., "mongodb://user:password@host:port/database") | |
| // If renaming is desired, document it as a breaking change; otherwise revert the name. | |
| public static CollectionManager createCollectionManagerFromURI(String uri) { |
| if (host.isEmpty()) { | ||
| builder.append("localhost"); | ||
| } else { | ||
| builder.append(host); |
There was a problem hiding this comment.
🟡 MEDIUM — Client closure in setup/getConn may break existing CollectionManager instances (medium confidence — verify before acting)
The new getConn method (line 264) and the fallback path in setup (line 225) now close the currently stored client and replace it with a new one. If multiple CollectionManager instances are created via the factory (e.g., in a server application with many managers), the old client becomes unusable, causing operations on previously created managers to fail. The old code did not close previous clients, allowing them to coexist. This regressive behaviour is not mentioned in the PR description or tested in the unit tests.
| @@ -164,67 +187,83 @@ public static CollectionManager setup(ServletContext context) { | |||
| if (props == null) { | |||
There was a problem hiding this comment.
🔵 LOW — Description gap: PR only mentions adding tests but includes functional changes (low confidence — verify before acting)
The PR description states 'add unit tests', but the diff includes many non-test changes: API rename, thread-safe client creation, client lifecycle changes, defensive copies in MongoQuery, and replacement of deprecated Class.newInstance(). The description does not mention any of these functional modifications.
No description provided.