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

Bring JUnit Jupiter Assumptions to RequestBoundaryMethodsTest to Avoid Test Case Quiet Quit #2382

Conversation

Codegass
Copy link
Contributor

@Codegass Codegass commented Apr 4, 2024

Resolve #2359

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.

PR Implementation

The change will be straightforward and involves replacing if statements with Assume.assumeTrue in all five test cases.

Reference Documentations/Specifications

Junit5 Assumption Doc

Copy link

codecov bot commented Apr 4, 2024

Codecov Report

Attention: Patch coverage is 67.02703% with 61 lines in your changes are missing coverage. Please review.

Project coverage is 49.83%. Comparing base (9a8849b) to head (4ccbfee).
Report is 20 commits behind head on main.

❗ Current head 4ccbfee differs from pull request most recent head 2dbd726. Consider uploading reports for the commit 2dbd726 to get more accurate results

Files Patch % Lines
...a/com/microsoft/sqlserver/jdbc/SQLServerError.java 32.14% 17 Missing and 2 partials ⚠️
...microsoft/sqlserver/jdbc/SQLServerInfoMessage.java 44.44% 14 Missing and 1 partial ⚠️
...m/microsoft/sqlserver/jdbc/SQLServerException.java 64.28% 5 Missing ⚠️
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 50.00% 3 Missing and 1 partial ⚠️
...oft/sqlserver/jdbc/SQLServerBulkCSVFileRecord.java 80.00% 3 Missing and 1 partial ⚠️
.../microsoft/sqlserver/jdbc/SQLServerConnection.java 84.00% 3 Missing and 1 partial ⚠️
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java 73.33% 2 Missing and 2 partials ⚠️
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java 33.33% 1 Missing and 1 partial ⚠️
...t/sqlserver/jdbc/SQLServerConnectionPoolProxy.java 50.00% 1 Missing ⚠️
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java 87.50% 0 Missing and 1 partial ⚠️
... and 2 more
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2382      +/-   ##
============================================
- Coverage     50.03%   49.83%   -0.20%     
- Complexity     3785     3822      +37     
============================================
  Files           143      145       +2     
  Lines         33204    33360     +156     
  Branches       5629     5654      +25     
============================================
+ Hits          16613    16625      +12     
- Misses        14207    14368     +161     
+ Partials       2384     2367      -17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lilgreenbird lilgreenbird added this to In progress in MSSQL JDBC via automation Apr 4, 2024
lilgreenbird
lilgreenbird previously approved these changes Apr 5, 2024
@Jeffery-Wasty Jeffery-Wasty moved this from In progress to Under Peer Review in MSSQL JDBC Apr 5, 2024
@Jeffery-Wasty Jeffery-Wasty modified the milestones: 12.7.1, 12.7.2 Apr 5, 2024
Jeffery-Wasty
Jeffery-Wasty previously approved these changes Apr 8, 2024
@Jeffery-Wasty Jeffery-Wasty self-requested a review April 8, 2024 19:09
@Codegass Codegass dismissed stale reviews from Jeffery-Wasty and lilgreenbird via 2dbd726 April 8, 2024 19:19
…oundaryMethodsTest.java

Co-authored-by: Jeff Wasty <v-jeffwasty@microsoft.com>
MSSQL JDBC automation moved this from Under Peer Review to In progress Apr 8, 2024
MSSQL JDBC automation moved this from In progress to Closed/Merged PRs Apr 10, 2024
@lilgreenbird lilgreenbird reopened this Apr 10, 2024
MSSQL JDBC automation moved this from Closed/Merged PRs to In progress Apr 10, 2024
@lilgreenbird lilgreenbird merged commit 193edbd into microsoft:main Apr 10, 2024
9 of 18 checks passed
MSSQL JDBC automation moved this from In progress to Closed/Merged PRs Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
MSSQL JDBC
  
Closed/Merged PRs
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Bring JUnit Jupiter Assumptions to RequestBoundaryMethodsTest to Avoid Test Case Quiet Quit
4 participants