Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancing Clarity with assumeTrue in Test Precondition Checks for validatePropertyUpdates and validateApiWhenRemovingChild #712

Closed
Codegass opened this issue Mar 18, 2024 · 0 comments · Fixed by #713

Comments

@Codegass
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant