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

[FEATURE REQUEST] Bring JUnit Jupiter Assumptions to RequestBoundaryMethodsTest to Avoid Test Case Quiet Quit #2359

Closed
Codegass opened this issue Mar 21, 2024 · 4 comments · Fixed by #2382
Labels
Enhancement An enhancement to the driver. Lower priority than bugs.
Projects

Comments

@Codegass
Copy link
Contributor

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

When I am go through the test class RequestBoundaryMethodsTest, I noticed that many of the test cases in this test class is using the if condition to determine if a test should proceed based on certain preconditions.

Here is an example from testWarnings:

    @Test
    public void testWarnings() throws SQLException {
        try (Connection con = getConnection()) {
            if (TestUtils.isJDBC43OrGreater(con)) { \\ If Condition is false, then the whole test will quietly finished
                con.beginRequest();
                generateWarning(con);
                assertNotNull(con.getWarnings());
                ...
                ...
            }
        }
    }

This approach can lead to tests passing under conditions where they're not actually validating the intended functionality.

As I checked, all these test cases shared this design pattern in the RequestBoundaryMethodsTest :

Describe the preferred solution

I would like to propose we adopt the JUnit Jupiter's assumption API to handle such precondition checks. The assumption API allows tests to be skipped when certain conditions are not met, making it clear that the test result is not applicable under those conditions. So here we can use the assumption api to replace the if condition without quiet quit. (If the assumption is false the TestAbortedException will be recorded).

For instance, the testWarnings case can be refactored as follows:

@Test
public void testWarnings() throws SQLException {
    try (Connection con = getConnection()) {
        Assume.assumeTrue(TestUtils.isJDBC43OrGreater(con));

        con.beginRequest();
        generateWarning(con);
        ...
        ...
}

This change will make test results more accurately reflect the behavior being tested, as irrelevant tests are skipped rather than falsely passing.

Implementation

The change will be straightforward and involves replacing if statements with Assume.assumeTrue (or other relevant Assume methods) where appropriate for all these test cases

Describe alternatives you've considered

Additional context

Reference Documentations/Specifications

Junit5 Assumption Doc

Reference Implementation


If you feel this suggestion is helpful, I am more than happy to submit a PR to implement the refactoring.

@lilgreenbird
Copy link
Member

hi @Codegass

yes that would be an improvement however this would be a lower priority item for us so if you could submit a PR that would definitely speed things up we would be happy to get it reviewed. Thanks!

@lilgreenbird lilgreenbird added the Enhancement An enhancement to the driver. Lower priority than bugs. label Mar 21, 2024
@lilgreenbird lilgreenbird added this to Under Investigation in MSSQL JDBC via automation Mar 21, 2024
@lilgreenbird
Copy link
Member

closing, please go ahead and submit a PR your convenience it will go through the review process

MSSQL JDBC automation moved this from Under Investigation to Closed Issues Mar 27, 2024
@Codegass
Copy link
Contributor Author

Thanks, I'll submit the PR ASAP.

@Jeffery-Wasty
Copy link
Member

Reopening since there is now a PR to address this.

@Jeffery-Wasty Jeffery-Wasty reopened this Apr 5, 2024
MSSQL JDBC automation moved this from Closed Issues to Under Investigation Apr 5, 2024
MSSQL JDBC automation moved this from Under Investigation to Closed Issues Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement An enhancement to the driver. Lower priority than bugs.
Projects
MSSQL JDBC
  
Closed Issues
3 participants