Add unit tests to improve test coverage#57
Merged
orpiske merged 2 commits intowanaku-ai:mainfrom Jan 31, 2026
Merged
Conversation
- PropertyFileProviderTest: file loading, null URI handling, error cases - FileConfigurationWriterTest: file writing, URI generation, error handling - DiscoveryHelperTest: address resolution with 'auto' keyword support - TokenEndpointTest: URL construction strategies and autoResolve logic Addresses wanaku-ai#55
Reviewer's GuideAdds JUnit 5 as a test dependency to the discovery and security modules and introduces comprehensive unit tests for file-based configuration providers, discovery address resolution, and token endpoint URL construction/auto-resolution logic to significantly improve coverage. Class diagram for new test coverage relationshipsclassDiagram
class PropertyFileProvider
class FileConfigurationWriter
class DiscoveryHelper
class TokenEndpoint
class PropertyFileProviderTest
class FileConfigurationWriterTest
class DiscoveryHelperTest
class TokenEndpointTest
PropertyFileProviderTest --> PropertyFileProvider : tests
FileConfigurationWriterTest --> FileConfigurationWriter : tests
DiscoveryHelperTest --> DiscoveryHelper : tests
TokenEndpointTest --> TokenEndpoint : tests
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
TokenEndpointTestexpectations for URLs with trailing slashes and emptytokenEndpointUri(e.g., the double//and/protocol/openid-connect/tokenwithout a base) look brittle; consider normalizing URL joining inTokenEndpointand updating tests to assert the normalized behavior instead of relying on these edge-case string concatenations. - The
DiscoveryHelperTestmethods that assert on IP address format (resolveRegistrationAddressResolvesAutoKeywordand related) may be environment-sensitive (hostnames vs. IPs, IPv4 vs. IPv6); consider loosening these assertions or isolating them from specific format assumptions to avoid flakiness across different CI/runtime environments. - In
FileConfigurationWriterTest.writesContentWithUnicodeCharacters, you only assert file existence; it would be more robust to also assert that the file contents match the original Unicode string to fully verify encoding/decoding behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `TokenEndpointTest` expectations for URLs with trailing slashes and empty `tokenEndpointUri` (e.g., the double `//` and `/protocol/openid-connect/token` without a base) look brittle; consider normalizing URL joining in `TokenEndpoint` and updating tests to assert the normalized behavior instead of relying on these edge-case string concatenations.
- The `DiscoveryHelperTest` methods that assert on IP address format (`resolveRegistrationAddressResolvesAutoKeyword` and related) may be environment-sensitive (hostnames vs. IPs, IPv4 vs. IPv6); consider loosening these assertions or isolating them from specific format assumptions to avoid flakiness across different CI/runtime environments.
- In `FileConfigurationWriterTest.writesContentWithUnicodeCharacters`, you only assert file existence; it would be more robust to also assert that the file contents match the original Unicode string to fully verify encoding/decoding behavior.
## Individual Comments
### Comment 1
<location> `capabilities-config-providers/capabilities-config-provider-file/src/test/java/ai/wanaku/capabilities/sdk/config/provider/file/FileConfigurationWriterTest.java:87-96` </location>
<code_context>
+ }
+
+ @Test
+ void writesContentWithUnicodeCharacters() throws IOException {
+ FileConfigurationWriter writer = new FileConfigurationWriter(tempDir.toFile());
+ String unicodeData = "key=日本語\nother=émoji🎉";
+
+ URI uri = writer.write("unicode.txt", unicodeData);
+
+ Path writtenFile = Path.of(uri);
+ assertTrue(Files.exists(writtenFile));
+ }
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the Unicode test by asserting round-trip content equality, not just file existence
This test currently only checks that a file exists, which duplicates coverage from other tests. To validate Unicode handling (including emojis and other non-ASCII characters), please also assert the written contents, e.g. `assertEquals(unicodeData, Files.readString(writtenFile));`, so encoding issues are caught rather than just file creation.
```suggestion
@Test
void writesContentWithUnicodeCharacters() throws IOException {
FileConfigurationWriter writer = new FileConfigurationWriter(tempDir.toFile());
String unicodeData = "key=日本語\nother=émoji🎉";
URI uri = writer.write("unicode.txt", unicodeData);
Path writtenFile = Path.of(uri);
assertTrue(Files.exists(writtenFile));
assertEquals(unicodeData, Files.readString(writtenFile));
}
```
</issue_to_address>
### Comment 2
<location> `capabilities-config-providers/capabilities-config-provider-file/src/test/java/ai/wanaku/capabilities/sdk/config/provider/file/FileConfigurationWriterTest.java:68-77` </location>
<code_context>
+ }
+
+ @Test
+ void throwsConfigWriteExceptionForInvalidDirectory() {
+ File nonExistentDir = new File("/non/existent/path/that/does/not/exist");
+ FileConfigurationWriter writer = new FileConfigurationWriter(nonExistentDir);
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for write failures on an existing but non-writable directory
If your CI setup allows, you could point the writer at a temporary directory made read-only and assert that `write` throws `ConfigWriteException`. That would validate the failure path during the actual write call rather than only at directory resolution time.
```suggestion
@Test
void throwsConfigWriteExceptionForInvalidDirectory() {
File nonExistentDir = new File("/non/existent/path/that/does/not/exist");
FileConfigurationWriter writer = new FileConfigurationWriter(nonExistentDir);
assertThrows(ConfigWriteException.class, () -> writer.write("file.txt", "data"));
}
@Test
void throwsConfigWriteExceptionForNonWritableDirectory() throws IOException {
Path readOnlyDir = Files.createTempDirectory("readOnlyDir");
File nonWritableDir = readOnlyDir.toFile();
assertTrue(nonWritableDir.setWritable(false, false), "Failed to make directory non-writable");
FileConfigurationWriter writer = new FileConfigurationWriter(nonWritableDir);
assertThrows(ConfigWriteException.class, () -> writer.write("file.txt", "data"));
}
@Test
void writesMultilineContent() throws IOException {
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...c/test/java/ai/wanaku/capabilities/sdk/config/provider/file/FileConfigurationWriterTest.java
Show resolved
Hide resolved
...c/test/java/ai/wanaku/capabilities/sdk/config/provider/file/FileConfigurationWriterTest.java
Show resolved
Hide resolved
- Add content assertion to Unicode test for round-trip verification - Add test for non-writable directory error handling - Loosen DiscoveryHelper IP format assertions to avoid CI flakiness
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Coverage Improvements
Addresses #55
Test plan
mvn clean test)Summary by Sourcery
Add unit tests to validate file-based configuration, discovery utilities, and token endpoint behavior, and wire up required test dependencies for the affected modules.
Build:
Tests: