I would like to propose a minor improvement that could apply to two test cases in DefaultLayeredConfigTest. The two test cases are validatePropertyUpdates and validateApiWhenRemovingChild, I will use validateApiWhenRemovingChild as an example. As in the test case, it utilizes assert statements for initial state validation, as shown below:
// Validate initial state
assertEquals("propvalue", config.getProperty("propname").get());
assertEquals("propvalue", config.getRawProperty("propname"));
While this approach checks for the expected conditions, it intertwines precondition checks with the test logic itself, which might not be ideal for cases where we want to distinguish between a test setup issue and an actual failure in the functionality being tested.
The assumeTrue method, as documented by JUnit5, offers a clearer alternative. It allows for stating assumptions about the test environment or preconditions, where a failed assumption results in the test being marked as inconclusive rather than a failure. This distinction is crucial for ensuring that tests only proceed under the correct conditions, thus avoiding misleading outcomes.
For instance, replacing our current initial state validation with assumeTrue would look like this:
// Validate initial state
assumeTrue("propvalue".equals(config.getProperty("propname").orElse(null)),
"Property 'propname' does not have the expected value 'propvalue', test assumptions not met.");
assumeTrue("propvalue".equals(config.getRawProperty("propname")),
"Raw property 'propname' does not have the expected value 'propvalue', test assumptions not met.");
I feel this adjustment will make the test's logic and its prerequisites more explicit, separating the precondition checks from the actual assertions. And if you don't mind I am willing to submit a PR to implement the suggestion. Thank you for considering this suggestion.
I would like to propose a minor improvement that could apply to two test cases in
DefaultLayeredConfigTest. The two test cases arevalidatePropertyUpdatesandvalidateApiWhenRemovingChild, I will usevalidateApiWhenRemovingChildas an example. As in the test case, it utilizes assert statements for initial state validation, as shown below:While this approach checks for the expected conditions, it intertwines precondition checks with the test logic itself, which might not be ideal for cases where we want to distinguish between a test setup issue and an actual failure in the functionality being tested.
The assumeTrue method, as documented by JUnit5, offers a clearer alternative. It allows for stating assumptions about the test environment or preconditions, where a failed assumption results in the test being marked as inconclusive rather than a failure. This distinction is crucial for ensuring that tests only proceed under the correct conditions, thus avoiding misleading outcomes.
For instance, replacing our current initial state validation with assumeTrue would look like this:
I feel this adjustment will make the test's logic and its prerequisites more explicit, separating the precondition checks from the actual assertions. And if you don't mind I am willing to submit a PR to implement the suggestion. Thank you for considering this suggestion.