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

Add Assertion to the SharedTimerTest #2381

Merged
merged 3 commits into from
May 13, 2024

Conversation

Codegass
Copy link
Contributor

@Codegass Codegass commented Apr 4, 2024

Resolve #2357

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 walking through the unit test of the mssql-jdbc, I noticed that currently the SharedTimerTest does not contain any assertion statement to verify the behavior and state of the SharedTimer after execution of its getTimer() method in a multithreaded environment. The test ensures that references are added and removed, but does not assert the expected state of the SharedTimer (e.g., it is stopped and cleaned up properly after all references are removed). This could potentially mask issues with how SharedTimer manages its lifecycle and resources.

Current Implementation:

@Test
    void getTimer() throws InterruptedException, ExecutionException, TimeoutException {
        final int iterations = 500;

        ExecutorService executor = Executors.newFixedThreadPool(2);
        try {
            ArrayList<CompletableFuture<?>> futures = new ArrayList<>(iterations);
            for (int i = 0; i < iterations; i++) {
                futures.add(CompletableFuture.runAsync(() -> SharedTimer.getTimer().removeRef(), executor));
            }

            CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).get(2, TimeUnit.MINUTES);
        } finally {
            executor.shutdown();
        }
    }

Describe the preferred solution

I'd like to propose a minor improvement by adding a simple assertion at the end of the getTimer test method to verify that the SharedTimer has indeed been stopped and its resources have been cleaned up after all references to it have been removed. This could be accomplished by asserting that SharedTimer.isRunning() returns false, indicating that the internal ScheduledThreadPoolExecutor has been shut down and the SharedTimer instance has been cleaned up. Here is the suggested code snippet for this improvement:

    @Test
    void getTimer() throws InterruptedException, ExecutionException, TimeoutException {
        final int iterations = 500;
        ExecutorService executor = Executors.newFixedThreadPool(2);

        try {
            ArrayList<CompletableFuture<?>> futures = new ArrayList<>(iterations);
            for (int i = 0; i < iterations; i++) {
                futures.add(CompletableFuture.runAsync(() -> SharedTimer.getTimer().removeRef(), executor));
            }

            CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).get(2, TimeUnit.MINUTES);
        } finally {
            executor.shutdown();
            if (!executor.awaitTermination(800, TimeUnit.MILLISECONDS)) {
                executor.shutdownNow();
            }
        }

        assertFalse(SharedTimer.isRunning(), "SharedTimer should be stopped after all references are removed.");
    }

Describe alternatives you've considered

An alternative would be to incorporate more detailed logging within the SharedTimer class regarding its state transitions (e.g., when references are added or removed, and when it starts or stops). While this would provide more insight during test execution, it would not replace the need for explicit assertions to verify the correct behavior programmatically.

Reference Documentations/Specifications

This suggestion does not directly relate to any specific JDBC Specifications.

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 50.21%. Comparing base (9a8849b) to head (c3894d1).
Report is 24 commits behind head on main.

❗ Current head c3894d1 differs from pull request most recent head 24944b8. Consider uploading reports for the commit 24944b8 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    #2381      +/-   ##
============================================
+ Coverage     50.03%   50.21%   +0.18%     
- Complexity     3785     3842      +57     
============================================
  Files           143      145       +2     
  Lines         33204    33360     +156     
  Branches       5629     5654      +25     
============================================
+ Hits          16613    16752     +139     
- Misses        14207    14210       +3     
- Partials       2384     2398      +14     

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

@Jeffery-Wasty Jeffery-Wasty modified the milestones: 12.7.1, 12.7.2 Apr 5, 2024
Copy link
Contributor

@lilgreenbird lilgreenbird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update as per review comments

@Jeffery-Wasty Jeffery-Wasty added the Under Review Used for pull requests under review label Apr 29, 2024
@Jeffery-Wasty Jeffery-Wasty removed the Under Review Used for pull requests under review label May 8, 2024
@lilgreenbird lilgreenbird merged commit ae64fed into microsoft:main May 13, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed/Merged PRs
Development

Successfully merging this pull request may close these issues.

Add Assertion to the SharedTimer Test
5 participants