Unify settings files by prioritizing mockk.properties in MockKSettings#1474
Unify settings files by prioritizing mockk.properties in MockKSettings#1474Raibaz merged 8 commits intomockk:masterfrom
Conversation
|
There is a problem with the Gemini CLI PR review. Please check the action logs for details. |
| val unifiedPropertiesStream = Thread.currentThread().contextClassLoader | ||
| .getResourceAsStream("mockk.properties") | ||
|
|
||
| if (unifiedPropertiesStream != null) { | ||
| unifiedPropertiesStream.use(properties::load) | ||
| } else { | ||
| // Fallback to the legacy settings.properties for backward compatibility | ||
| MockKSettings::class.java | ||
| .getResourceAsStream("settings.properties") | ||
| ?.use(properties::load) | ||
| } |
There was a problem hiding this comment.
How about compacting this logic by using the same class as entry point to the class path? We can leverage the resource path resolution, that is
- "/file" is read from the class path root
- "file" is read relative to this class (like the initial code)
Then with a scope function we can shorten it:
| val unifiedPropertiesStream = Thread.currentThread().contextClassLoader | |
| .getResourceAsStream("mockk.properties") | |
| if (unifiedPropertiesStream != null) { | |
| unifiedPropertiesStream.use(properties::load) | |
| } else { | |
| // Fallback to the legacy settings.properties for backward compatibility | |
| MockKSettings::class.java | |
| .getResourceAsStream("settings.properties") | |
| ?.use(properties::load) | |
| } | |
| MockKSettings::class.java.run { | |
| getResourceAsStream("/mockk.properties") | |
| // Fallback to the legacy file for backward compatibility | |
| ?: getResourceAsStream("settings.properties") | |
| }?.use(properties::load) |
There was a problem hiding this comment.
Thanks for the suggestion!
I considered this approach, but opted to fully encapsulate the loading logic in UnifiedPropertiesLoader instead of keeping it inline. This addresses your
broader concern about duplication with RestrictMockkConfiguration and DefaultPropertiesLoader.
Now MockKSettings simply delegates to UnifiedPropertiesLoader().loadProperties(), which centralizes the fallback mechanism and the path constants in one
place. This makes it easier to maintain and test.
Let me know if you prefer the inline approach—I can adjust if needed.
|
There is still one duplication remaining: |
@polarene Thanks for pointing out the duplication in I started refactoring before seeing your comment, so I initially missed that path duplication. I've now updated the implementation to address the issues you What's updated
All tests pass and the behavior matches previous versions. |
|
Thanks @snowykte0426, I'm reviewing the latest changes. |
README.md
Outdated
| ## Configuration File | ||
|
|
||
| To adjust parameters globally, there are a few settings you can specify in a resource file. | ||
| To adjust parameters globally, you can specify settings in a unified configuration file. |
There was a problem hiding this comment.
Unifying the configurations is a feature we pulled off, but as a reader I don't care about it. I just want the current information to state things as if they've always been that way.
| To adjust parameters globally, you can specify settings in a unified configuration file. | |
| To adjust parameters globally, you can specify settings in a configuration file. |
README.md
Outdated
|
|
||
| ### How to use: | ||
|
|
||
| 1. Create a `mockk.properties` file in `src/test/resources/`. |
There was a problem hiding this comment.
It's better to specify the whole path at once, so a user can create it easily by copying and pasting directly in the shell or IDE.
| 1. Create a `mockk.properties` file in `src/test/resources/`. | |
| 1. Create the configuration file at `src/test/resources/mockk.properties`. |
README.md
Outdated
| ### Legacy Configuration (Backward Compatibility) | ||
|
|
||
| For backward compatibility, MockK also supports the legacy configuration file location: | ||
| - `src/test/resources/io/mockk/settings.properties` | ||
|
|
||
| If both files exist, `mockk.properties` takes precedence. The legacy location is deprecated and may be removed in a future version. |
There was a problem hiding this comment.
A few style and wording suggestions:
| ### Legacy Configuration (Backward Compatibility) | |
| For backward compatibility, MockK also supports the legacy configuration file location: | |
| - `src/test/resources/io/mockk/settings.properties` | |
| If both files exist, `mockk.properties` takes precedence. The legacy location is deprecated and may be removed in a future version. | |
| ### Legacy Configuration | |
| For backward compatibility, MockK also supports the legacy configuration file: |
src/test/resources/io/mockk/settings.properties
If both files exist, `mockk.properties` takes precedence. The legacy location is deprecated and will be removed in a future version.
README.md
Outdated
| You can configure Restricted Mocking behavior using the `mockk.properties` file. | ||
|
|
||
| #### 1. Creating the `mockk.properties` File | ||
| You can configure Restricted Mocking behavior using the unified `mockk.properties` configuration file described in the [Configuration File](#configuration-file) section. |
There was a problem hiding this comment.
Same reason as above
| You can configure Restricted Mocking behavior using the unified `mockk.properties` configuration file described in the [Configuration File](#configuration-file) section. | |
| You can configure Restricted Mocking behavior using the `mockk.properties` configuration file described in the [Configuration File](#configuration-file) section. |
README.md
Outdated
| You can configure Restricted Mocking behavior using the unified `mockk.properties` configuration file described in the [Configuration File](#configuration-file) section. | ||
|
|
||
| Place the file in one of the following directories: | ||
| Add the following properties to your `src/test/resources/mockk.properties` file: |
There was a problem hiding this comment.
Better not duplicate paths, instead a link would be best.
| Add the following properties to your `src/test/resources/mockk.properties` file: | |
| Add the following properties to your [configuration file](#configuration-file): |
There was a problem hiding this comment.
The code around, and this test especially, were clearly written with an LLM, but don't worry: we have code reviews exactly for improving it before merging, we can make it better. 😉
Just remember to always double-check ai-generated code.
| class UnifiedPropertiesLoaderTest { | ||
|
|
There was a problem hiding this comment.
The subject under test (SUT) initialization is the same for every test (in addition to being stateless), so we can extract it here. The default JUnit lifecycle will instantiate the test class for every case, so it doesn't matter state-wise anyway.
| class UnifiedPropertiesLoaderTest { | |
| class UnifiedPropertiesLoaderTest { | |
| val loader = UnifiedPropertiesLoader() | |
| // Given: A loader with no properties files in the test resources | ||
| val loader = UnifiedPropertiesLoader() | ||
|
|
||
| // When: Loading properties | ||
| val properties = loader.loadProperties() | ||
|
|
||
| // Then: Should return empty properties without throwing an exception | ||
| assertTrue(properties.isEmpty, "Properties should be empty when no file exists") |
There was a problem hiding this comment.
This test is fine, we just don't need these comments.
| // Given: A loader with no properties files in the test resources | |
| val loader = UnifiedPropertiesLoader() | |
| // When: Loading properties | |
| val properties = loader.loadProperties() | |
| // Then: Should return empty properties without throwing an exception | |
| assertTrue(properties.isEmpty, "Properties should be empty when no file exists") | |
| val properties = loader.loadProperties() | |
| assertTrue(properties.isEmpty, "Properties should be empty when no file exists") |
| @Test | ||
| fun `UNIFIED_PROPERTIES_FILE constant has correct value`() { | ||
| assertEquals("mockk.properties", UnifiedPropertiesLoader.UNIFIED_PROPERTIES_FILE) | ||
| } | ||
|
|
||
| @Test | ||
| fun `LEGACY_PROPERTIES_FILE constant has correct value`() { | ||
| assertEquals("settings.properties", UnifiedPropertiesLoader.LEGACY_PROPERTIES_FILE) | ||
| } |
There was a problem hiding this comment.
These tests are completely unnecessary: they're not testing any useful behavior, and should be removed. In addition, the constants should be made private, or at least internal for test purposes.
| @Test | ||
| fun `loadProperties uses correct context class loader`() { | ||
| // Given: A loader instance | ||
| val loader = UnifiedPropertiesLoader() | ||
|
|
||
| // When: Loading properties | ||
| val properties = loader.loadProperties() | ||
|
|
||
| // Then: Should not throw an exception even if no files exist | ||
| // This verifies that the correct class loader is being used | ||
| assertTrue(properties != null, "Properties should not be null") | ||
| } |
There was a problem hiding this comment.
This case isn't testing anything of what it says because:
- the return type can never be null
- resource loading never throws an exception, apart from the path being null
- there still aren't any configuration files on the classpath, so the behavior is identical to the first test case
This is just plain LLM hallucination :)
To properly test the loading mechanism, we need to create the resource files needed for every case, and delete them afterward. This way we can effectively check the loading behavior with real but temporary files. We just need a few utilities to make the tests easy to read:
| @Test | |
| fun `loadProperties uses correct context class loader`() { | |
| // Given: A loader instance | |
| val loader = UnifiedPropertiesLoader() | |
| // When: Loading properties | |
| val properties = loader.loadProperties() | |
| // Then: Should not throw an exception even if no files exist | |
| // This verifies that the correct class loader is being used | |
| assertTrue(properties != null, "Properties should not be null") | |
| } | |
| @Test | |
| fun `read default config`() { | |
| writeToClasspath(UnifiedPropertiesLoader.UNIFIED_PROPERTIES_FILE, "name=default") | |
| val config = loader.loadProperties() | |
| assertEquals(config["name"], "default") | |
| deleteFromClasspath(UnifiedPropertiesLoader.UNIFIED_PROPERTIES_FILE) | |
| } | |
| @Test | |
| fun `read legacy config`() { | |
| writeToClasspath(UnifiedPropertiesLoader.LEGACY_PROPERTIES_FILE, "name=legacy") | |
| val config = loader.loadProperties() | |
| assertEquals(config["name"], "legacy") | |
| deleteFromClasspath(UnifiedPropertiesLoader.LEGACY_PROPERTIES_FILE) | |
| } | |
| @Test | |
| fun `default config takes precedence over legacy`() { | |
| writeToClasspath(UnifiedPropertiesLoader.UNIFIED_PROPERTIES_FILE, "name=default") | |
| writeToClasspath(UnifiedPropertiesLoader.LEGACY_PROPERTIES_FILE, "name=legacy") | |
| val config = loader.loadProperties() | |
| assertEquals(config["name"], "default") | |
| deleteFromClasspath(UnifiedPropertiesLoader.UNIFIED_PROPERTIES_FILE) | |
| deleteFromClasspath(UnifiedPropertiesLoader.LEGACY_PROPERTIES_FILE) | |
| } | |
| fun writeToClasspath(path: String, content: String) { | |
| getRoot().resolve(path.removePrefix("/")).run { | |
| createParentDirectories() | |
| writeText(content) | |
| } | |
| } | |
| fun deleteFromClasspath(path: String) { | |
| getRoot().resolve(path.removePrefix("/")).deleteExisting() | |
| } | |
| fun getRoot(): Path = Path.of(this::class.java.getResource("/")!!.toURI()) |
| * This loader attempts to load properties from [UNIFIED_PROPERTIES_FILE] at the classpath root first. | ||
| * If not found, it falls back to the legacy [LEGACY_PROPERTIES_FILE] location for backward compatibility. | ||
| */ | ||
| class UnifiedPropertiesLoader : PropertiesLoader { |
There was a problem hiding this comment.
Last thing: since this class is stateless, consider changing it to object.
|
Thank you for the detailed review! I've addressed all your feedback. Changes MadeI refactored the implementation based on your suggestions: UnifiedPropertiesLoader.kt:
UnifiedPropertiesLoaderTest.kt:
README.md:
Additional RefactoringTo fully eliminate the duplication you identified in Removed deprecated files:
Question: I assumed these were internal-only since they were deprecated, but I realize Note on the TestsI apologize for not catching the AI-generated code in my initial implementation. I used an LLM to help with some parts and focused on the core logic, but I The new tests are completely rewritten with proper file I/O testing. Let me know if any adjustments are needed! |
|
I've updated the test implementation to use So I used the standard Java NIO |
|
Can you please rebase on top of the |
Fixes #1468 Signed-off-by: Kim Tae Eun <snowykte0426@naver.com>
Addresses polarene's feedback on #1474 - Create UnifiedPropertiesLoader in mockk-core as single source of truth - Refactor MockKSettings to use UnifiedPropertiesLoader - Refactor DefaultPropertiesLoader to delegate to UnifiedPropertiesLoader - Eliminate "mockk.properties" path duplication (2 occurrences → 1 constant) - Update README.md to document unified configuration approach - Add comprehensive tests for UnifiedPropertiesLoader - Update API declarations for mockk-core and mockk modules This eliminates the duplication identified in code review where both MockKSettings and DefaultPropertiesLoader had separate implementations for loading mockk.properties. All configuration loading now flows through a single unified mechanism while maintaining backward compatibility. Fixes #1468
- Fix critical path resolution bug in UnifiedPropertiesLoader
* Use absolute paths with leading '/' for correct classpath resolution
* Consolidate ClassLoader usage to single instance
* Refactor loadProperties() to idiomatic Kotlin with scope functions
- Convert UnifiedPropertiesLoader to singleton object
* Class is stateless, object pattern is more appropriate
* Update MockKSettings to use object reference
- Rewrite UnifiedPropertiesLoaderTest with comprehensive coverage
* Add tests for actual file loading from unified location
* Add tests for fallback to legacy location
* Add test for precedence (unified over legacy)
* Remove AI-generated tests with weak assertions
* Use kotlin.test instead of JUnit-specific imports
- Simplify README documentation
* Remove implementation detail references ("unified")
* Improve file path clarity for copy-paste usage
* Use link references to avoid duplication
* Strengthen deprecation notice for legacy location
Addresses all feedback from @polarene's review.
Fixes fallback mechanism that was broken due to relative path usage.
Signed-off-by: Kim Tae Eun <snowykte0426@naver.com>
Replace java.io.File API with kotlin.io.path extensions and Java NIO as suggested in code review. Uses Files.createDirectories() for parent directory creation since kotlin.io.path.createParentDirectories() is not available in the current Kotlin stdlib version. All tests pass with this implementation. Signed-off-by: Kim Tae Eun <snowykte0426@naver.com>
@Raibaz I've rebased on top of the latest master (58750d3) after #1483 merge. Resolved the Spotless formatting conflicts and applied all formatting |
|
Fixed CI failures in fd9fbd5 - replaced The tests were using I switched to Java NIO APIs ( Let me know your preference and I can adjust accordingly. |
|
We have to maintain backwards compatibility with 1.7.*, yes. |
- Replace kotlin.io.path.writeText with Files.writeString - Replace kotlin.io.path.deleteExisting with Files.delete - kotlin.io.path extensions require Kotlin 1.8.0+ - CI tests with Kotlin 1.7.22 were failing due to missing APIs Fixes CI failures in Check [java=21, kotlin=1.7.22] and similar jobs
|
Hi! Catching up now, thanks for the updates. Let me review the changes again. |
| import kotlin.test.assertTrue | ||
|
|
||
| class UnifiedPropertiesLoaderTest { | ||
| private val loader = UnifiedPropertiesLoader |
There was a problem hiding this comment.
This property is useless now that UnifiedPropertiesLoader is an object, consider inlining it.
| private fun writeToClasspath( | ||
| path: String, | ||
| content: String, | ||
| ) { |
There was a problem hiding this comment.
No need to format like this, the signature fits on one line
| private fun writeToClasspath( | |
| path: String, | |
| content: String, | |
| ) { | |
| private fun writeToClasspath(path: String, content: String) { |
There was a problem hiding this comment.
The review feedback suggested formatting the signature on one line, but Spotless automatically formats it as:
private fun writeToClasspath(
path: String,
content: String,
) { @@ -39,7 +39,10 @@
········deleteFromClasspath(UnifiedPropertiesLoader.LEGACY_PROPERTIES_FILE)
····}
-····private·fun·writeToClasspath(path:·String,·content:·String)·{
+····private·fun·writeToClasspath(
+········path:·String,
+········content:·String,
+····)·{
········resolveFromClasspath(path).run·{
············parent?.let·{·Files.createDirectories(it)·}
············Files.writeString(this,·content)
This formatting conflict prevents the CI from passing. Should we keep the Spotless format or override it to match the review preference?
@polarene
There was a problem hiding this comment.
Hello! Oh right, I didn't remember Raibaz introduced Spotless just recently. Sure, if the formatting is enforced with that style so be it, ignore my style suggestion.
| ) { | ||
| getRoot().resolve(path.removePrefix("/")).run { | ||
| parent?.let { Files.createDirectories(it) } | ||
| Files.writeString(this, content, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING) |
There was a problem hiding this comment.
These options are included by default when not passed, so we can simplify the call
| Files.writeString(this, content, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING) | |
| Files.writeString(this, content) |
| } | ||
|
|
||
| private fun deleteFromClasspath(path: String) { | ||
| Files.delete(getRoot().resolve(path.removePrefix("/"))) |
There was a problem hiding this comment.
I realized the expression getRoot().resolve(path.removePrefix("/")) is duplicated so it would be better to extract it to another function, for example resolveFromClasspath(path: String).
| Files.delete(getRoot().resolve(path.removePrefix("/"))) | ||
| } | ||
|
|
||
| private fun getRoot(): Path = Path.of(this::class.java.getResource("/")!!.toURI()) |
There was a problem hiding this comment.
There's a shorter extension property available:
| private fun getRoot(): Path = Path.of(this::class.java.getResource("/")!!.toURI()) | |
| private fun getRoot(): Path = Path.of(javaClass.getResource("/")!!.toURI()) |
- Inline loader property since UnifiedPropertiesLoader is now an object - Simplify method signatures to single line - Remove unnecessary StandardOpenOption parameters (default behavior) - Extract duplicated path resolution to resolveFromClasspath() - Use javaClass extension instead of this::class.java All tests pass.
@snowykte0426 thanks for your updates, everything seems good now. About Sorry for the incompatible suggestions regarding kotlin.io.path. I didn't know about the 1.7 compatibility, I always use the latest Kotlin. |
|
@polarene
All tests are passing (4/4). Please let me know if you’d like any further adjustments. |
|
CI failed: https://github.com/mockk/mockk/actions/runs/20718814279/job/59562044513?pr=1474 The apiCheck failed because UnifiedPropertiesLoader was changed to an object, which modified the public API. Fixed by updating the API dump.(6d14427) |
|
Thank you both, @snowykte0426 and @polarene ! |
Summary
This PR addresses #1468 by unifying MockK's configuration files into a single location, which should help streamline the configuration experience for users.
Problem
Currently, MockK uses two separate configuration files:
This dual-file approach has caused some confusion about where to configure different settings.
Solution
I've modified MockKSettings.kt to prioritize loading from mockk.properties at the classpath root, with a fallback to the legacy io/mockk/settings.properties
location for backward compatibility.
Changes
a. First attempt to load from mockk.properties (unified location)
b. Fall back to io/mockk/settings.properties if not found (legacy support)
Benefits
Testing
The change maintains compatibility with existing test suites:
Future Considerations (Optional)
If the maintainers think it would be valuable, I'd be happy to follow up with:
Please let me know if you'd like me to tackle any of these in this PR or in a follow-up!
Fixes #1468