microsoft/mssql-jdbc

[FEATURE REQUEST] Enhancing `WarningTest.testWarnings` Readability and Maintainability with Clearer Assert Logic

Codegass opened this issue · 1 comments

Is your feature request related to a problem? If so, please give a short summary of the problem and how the feature would resolve it

While running unit tests for the mssql-jdbc project, I've encountered a unit test case WarningTest.testWarnings which contains obscured logic, notably when asserting conditions within a loop combined with conditional logic. The assertion value's generating processing is indirect. It takes me some time to understand the unit test. So I think this test case may need some improvement to increase the clarity and comprehensibility of what is being asserted, making it easier to understand and maintain.

Current Implementation:

    @Test
    public void testWarnings() throws SQLException {
        try (Connection conn = getConnection()) {

            Properties info = conn.getClientInfo();
            conn.setClientInfo(info);
            SQLWarning warn = conn.getWarnings();
            assertEquals(null, warn, "Warnings found.");

            Properties info2 = new Properties();
            String[] infoArray = {"prp1", "prp2", "prp3", "prp4", "prp5"};
            for (int i = 0; i < 5; i++) {
                info2.put(infoArray[i], "");
            }
            conn.setClientInfo(info2);
            warn = conn.getWarnings();
            for (int i = 0; i < 5; i++) {
                boolean found = false;
                List<String> list = Arrays.asList(infoArray);
                for (String word : list) {
                    if (warn.toString().contains(word)) {
                        found = true;
                        break;
                    }
                }
                assertTrue(found, TestResource.getResource("R_warningsNotFound") + ": " + warn.toString());
                warn = warn.getNextWarning();
                found = false;
            }
            conn.clearWarnings();

            conn.setClientInfo("prop7", "");
            warn = conn.getWarnings();
            assertTrue(warn.toString().contains("prop7"), TestResource.getResource("R_warningsNotFound"));
            warn = warn.getNextWarning();
            assertEquals(null, warn, TestResource.getResource("R_warningsFound"));
        } catch (Exception e) {
            fail(TestResource.getResource("R_unexpectedErrorMessage") + e.getMessage());
        }
    }

Describe the preferred solution

I would like to propose enhancing the readability and maintainability of unit tests by simplifying assert blocks. Specifically, by employing Hamcrest matchers for assertions involving collections and streamlining logic that currently relies on loops and conditional statements. This approach will make assertions more declarative, expressing the intent more clearly and making tests easier to read and understand at a glance.

The refactoring draft is as following:

    @Test
    public void testWarnings() throws SQLException {
        try (Connection conn = getConnection()) {

            Properties info = conn.getClientInfo();
            conn.setClientInfo(info);
            SQLWarning warn = conn.getWarnings();
            assertThat(warn, is(nullValue()));

            Properties info2 = new Properties();
            List<String> infoList = Arrays.asList("prp1", "prp2", "prp3", "prp4", "prp5");
            infoList.forEach(prp -> info2.put(prp, ""));
            conn.setClientInfo(info2);
            warn = conn.getWarnings();

            infoList.forEach(item -> {
                assertThat(warn.toString(), anyOf(containsString(item)));
                warn = warn.getNextWarning();
            });
            conn.clearWarnings();

            conn.setClientInfo("prop7", "");
            warn = conn.getWarnings();
            assertThat(warn.toString(), containsString("prop7"));
            warn = warn.getNextWarning();
            assertThat(warn, is(nullValue()));
        } catch (Exception e) {
            fail(TestResource.getResource("R_unexpectedErrorMessage") + e.getMessage());
        }
    }

Key Changes:

  • Replace manual loops and conditions with Hamcrest matchers: Use Hamcrest's expressive matchers like hasItem or anyOf for more readable and concise assertions.
  • Leverage Java Stream API for collections: Utilize the Stream API to simplify operations on collections and enhance clarity, particularly for checks involving multiple conditions or elements.

Describe alternatives you've considered

Additional context

Reference Documentations/Specifications

Reference Implementation


If this suggestion is helpful, I am more than happy to submit a Pull Request to implement it.

I think we would rather keep this simple and not add any additional dependencies on 3rdparty frameworks.