This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 8.5.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
View the commit online: https://github.com/apache/tomcat/commit/c4c14207035ac95c99f7c2da1dc326e3e989efa6 commit c4c14207035ac95c99f7c2da1dc326e3e989efa6 Author: Mark Thomas <ma...@apache.org> AuthorDate: Wed Nov 20 13:21:51 2019 +0000 Fix SpotBugs issues in JDBC pool tests --- .../java/org/apache/tomcat/jdbc/bugs/Bug53367.java | 6 +-- .../apache/tomcat/jdbc/test/ConnectCountTest.java | 4 +- .../org/apache/tomcat/jdbc/test/FairnessTest.java | 4 +- .../apache/tomcat/jdbc/test/JmxPasswordTest.java | 2 +- .../apache/tomcat/jdbc/test/MultipleCloseTest.java | 4 +- .../apache/tomcat/jdbc/test/StarvationTest.java | 2 + .../org/apache/tomcat/jdbc/test/TestException.java | 12 ++--- .../tomcat/jdbc/test/TestStatementCache.java | 2 + .../org/apache/tomcat/jdbc/test/TestTimeout.java | 26 +++++++---- .../jdbc/test/TestValidationQueryTimeout.java | 16 +++---- res/findbugs/filter-false-positives.xml | 51 ++++++++++++++++++++++ 11 files changed, 99 insertions(+), 30 deletions(-) diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/bugs/Bug53367.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/bugs/Bug53367.java index 1e534e0..6c0984e 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/bugs/Bug53367.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/bugs/Bug53367.java @@ -107,8 +107,8 @@ public class Bug53367 { Thread thread = new Thread(new Runnable() { @Override public void run() { - try { - ds.getConnection(); + // Expected to fail + try (Connection c = ds.getConnection()) { } catch (Exception e) { System.err.println("Step 2:"+e.getMessage()); } @@ -174,4 +174,4 @@ public class Bug53367 { Assert.assertEquals(0, pool.getActive()); Assert.assertEquals(threadsCount, pool.getSize()); } -} \ No newline at end of file +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/ConnectCountTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/ConnectCountTest.java index 5a0a71f..8e240f9 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/ConnectCountTest.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/ConnectCountTest.java @@ -17,7 +17,9 @@ package org.apache.tomcat.jdbc.test; import java.sql.Connection; +import java.sql.SQLException; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; @@ -259,7 +261,7 @@ public class ConnectCountTest extends DefaultTestCase { totalruntime+=(System.nanoTime()-start); } - } catch (Exception x) { + } catch (RuntimeException | SQLException | ExecutionException | InterruptedException x) { x.printStackTrace(); } finally { ConnectCountTest.this.latch.countDown(); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/FairnessTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/FairnessTest.java index 1c282df..bb2cc8b 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/FairnessTest.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/FairnessTest.java @@ -17,7 +17,9 @@ package org.apache.tomcat.jdbc.test; import java.sql.Connection; +import java.sql.SQLException; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; @@ -241,7 +243,7 @@ public class FairnessTest extends DefaultTestCase { totalruntime+=(System.nanoTime()-start); } - } catch (Exception x) { + } catch (RuntimeException | SQLException | ExecutionException | InterruptedException x) { x.printStackTrace(); } finally { FairnessTest.this.latch.countDown(); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/JmxPasswordTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/JmxPasswordTest.java index 2074447..6c05c00 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/JmxPasswordTest.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/JmxPasswordTest.java @@ -36,7 +36,7 @@ import org.apache.tomcat.jdbc.test.driver.Driver; public class JmxPasswordTest extends DefaultTestCase{ public static final String password = "password"; public static final String username = "username"; - public static ObjectName oname = null; + public ObjectName oname = null; @Before public void setUp() throws Exception { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/MultipleCloseTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/MultipleCloseTest.java index cd867d3..4180c51 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/MultipleCloseTest.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/MultipleCloseTest.java @@ -61,9 +61,11 @@ public class MultipleCloseTest extends DefaultTestCase { Assert.assertTrue(con1.isClosed()); // Open a new connection (This will re-use the previous pooled connection) - datasource.getConnection(); + Connection con2 = datasource.getConnection(); // A connection, once closed, should stay closed Assert.assertTrue(con1.isClosed()); + + con2.close(); } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/StarvationTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/StarvationTest.java index 0c1a667..1c551ed 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/StarvationTest.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/StarvationTest.java @@ -82,6 +82,7 @@ public class StarvationTest extends DefaultTestCase { }finally { if (con2!=null) con2.close(); } + con1.close(); } @Test @@ -104,5 +105,6 @@ public class StarvationTest extends DefaultTestCase { }finally { if (con2!=null) con2.close(); } + con1.close(); } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestException.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestException.java index be06e38..fe4d20d 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestException.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestException.java @@ -17,7 +17,9 @@ package org.apache.tomcat.jdbc.test; import java.sql.Connection; +import java.sql.Statement; +import org.junit.Assert; import org.junit.Test; import org.apache.tomcat.jdbc.pool.ConnectionPool; @@ -30,11 +32,11 @@ public class TestException extends DefaultTestCase { public void testException() throws Exception { datasource.getPoolProperties().setJdbcInterceptors(TestInterceptor.class.getName()); Connection con = datasource.getConnection(); - try { - con.createStatement(); - }catch (Exception x) { - // Ignore + try (Statement s = con.createStatement()){ + } catch (Exception x) { + Assert.fail(); } + con.close(); } @@ -42,7 +44,7 @@ public class TestException extends DefaultTestCase { @Override public void reset(ConnectionPool parent, PooledConnection con) { - // TODO Auto-generated method stub + // NO-OP } } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestStatementCache.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestStatementCache.java index 3a8778a..a80962c 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestStatementCache.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestStatementCache.java @@ -88,6 +88,7 @@ public class TestStatementCache extends DefaultTestCase { ps3.close(); Assert.assertTrue(ps3.isClosed()); Assert.assertEquals(1,interceptor.getCacheSize().get()); + con.close(); } @Test @@ -108,6 +109,7 @@ public class TestStatementCache extends DefaultTestCase { ps3.close(); Assert.assertTrue(ps3.isClosed()); Assert.assertEquals(0,interceptor.getCacheSize().get()); + con.close(); } @Test diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestTimeout.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestTimeout.java index 9b9fbde..aec85b6 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestTimeout.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestTimeout.java @@ -16,17 +16,18 @@ */ package org.apache.tomcat.jdbc.test; -import java.util.concurrent.atomic.AtomicInteger; +import java.sql.Connection; +import java.util.HashSet; +import java.util.Set; import org.junit.Assert; import org.junit.Test; public class TestTimeout extends DefaultTestCase { - AtomicInteger counter = new AtomicInteger(0); - @Test public void testCheckoutTimeout() throws Exception { + Set<Connection> cons = new HashSet<>(); try { this.datasource.getPoolProperties().setTestWhileIdle(true); this.datasource.getPoolProperties().setTestOnBorrow(false); @@ -42,20 +43,24 @@ public class TestTimeout extends DefaultTestCase { System.out.println("About to test connection pool:"+datasource); for (int i = 0; i < 21; i++) { long now = System.currentTimeMillis(); - this.datasource.getConnection(); + cons.add(this.datasource.getConnection()); long delta = System.currentTimeMillis()-now; System.out.println("Got connection #"+i+" in "+delta+" ms."); } - Assert.assertTrue(false); + Assert.fail(); } catch ( Exception x ) { - Assert.assertTrue(true); + // Expected on 21st checkout }finally { Thread.sleep(2000); } + for (Connection c : cons) { + c.close(); + } } @Test public void testCheckoutTimeoutFair() throws Exception { + Set<Connection> cons = new HashSet<>(); try { this.datasource.getPoolProperties().setFairQueue(true); this.datasource.getPoolProperties().setTestWhileIdle(true); @@ -72,15 +77,18 @@ public class TestTimeout extends DefaultTestCase { System.out.println("About to test connection pool:"+datasource); for (int i = 0; i < 21; i++) { long now = System.currentTimeMillis(); - this.datasource.getConnection(); + cons.add(this.datasource.getConnection()); long delta = System.currentTimeMillis()-now; System.out.println("Got connection #"+i+" in "+delta+" ms."); } - Assert.assertTrue(false); + Assert.fail(); } catch ( Exception x ) { - Assert.assertTrue(true); + // Expected on 21st checkout }finally { Thread.sleep(2000); } + for (Connection c : cons) { + c.close(); + } } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestValidationQueryTimeout.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestValidationQueryTimeout.java index 79e2fdc..054df27 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestValidationQueryTimeout.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestValidationQueryTimeout.java @@ -35,7 +35,7 @@ import org.apache.tomcat.jdbc.pool.interceptor.QueryTimeoutInterceptor; public class TestValidationQueryTimeout extends DefaultTestCase { - private static int TIMEOUT = 10; + private static final int TIMEOUT = 10; private static boolean isTimeoutSet; private static final String longQuery = "select * from test as A, test as B, test as C, test as D, test as E"; @@ -54,7 +54,6 @@ public class TestValidationQueryTimeout extends DefaultTestCase { this.datasource.setValidationQuery("SELECT 1"); this.datasource.setValidationQueryTimeout(TIMEOUT); - TIMEOUT = 10; isTimeoutSet = false; } @@ -67,8 +66,9 @@ public class TestValidationQueryTimeout extends DefaultTestCase { @Test public void testValidationQueryTimeoutEnabled() throws Exception { // because testOnBorrow is true, this triggers the validation query - this.datasource.getConnection(); + Connection con = this.datasource.getConnection(); Assert.assertTrue(isTimeoutSet); + con.close(); } @Test @@ -76,8 +76,9 @@ public class TestValidationQueryTimeout extends DefaultTestCase { this.datasource.setValidationQueryTimeout(-1); // because testOnBorrow is true, this triggers the validation query - this.datasource.getConnection(); + Connection con = this.datasource.getConnection(); Assert.assertFalse(isTimeoutSet); + con.close(); } @Test @@ -91,9 +92,6 @@ public class TestValidationQueryTimeout extends DefaultTestCase { Connection con = this.datasource.getConnection(); Assert.assertTrue(isTimeoutSet); - // increase the expected timeout to 30, which is what we set for the interceptor - TIMEOUT = 30; - // now create a statement, make sure the query timeout is set by the interceptor Statement st = con.createStatement(); Assert.assertEquals(interceptorTimeout, st.getQueryTimeout()); @@ -107,10 +105,10 @@ public class TestValidationQueryTimeout extends DefaultTestCase { con.close(); // pull another connection and check it - TIMEOUT = 10; isTimeoutSet = false; - this.datasource.getConnection(); + Connection con2 = this.datasource.getConnection(); Assert.assertTrue(isTimeoutSet); + con2.close(); } // this test depends on the execution time of the validation query diff --git a/res/findbugs/filter-false-positives.xml b/res/findbugs/filter-false-positives.xml index 73443d6..06f104e 100644 --- a/res/findbugs/filter-false-positives.xml +++ b/res/findbugs/filter-false-positives.xml @@ -1898,6 +1898,15 @@ <Bug code="ST" /> </Match> <Match> + <!-- The name shadowing is deliberate --> + <Or> + <Class name="org.apache.tomcat.jdbc.test.driver.Connection" /> + <Class name="org.apache.tomcat.jdbc.test.driver.Driver" /> + <Class name="org.apache.tomcat.jdbc.test.driver.ResultSet" /> + </Or> + <Bug pattern="NM_SAME_SIMPLE_NAME_AS_INTERFACE" /> + </Match> + <Match> <!-- The call with the ignored return value is used to ensure the pool --> <!-- thinks the connection is being used. --> <Class name="org.apache.tomcat.jdbc.test.AbandonPercentageTest" /> @@ -1905,6 +1914,32 @@ <Bug pattern="RV_RETURN_VALUE_IGNORED" /> </Match> <Match> + <!-- A number of the tests incude performance tests --> + <Class name="org.apache.tomcat.jdbc.test.DefaultTestCase" /> + <Method name="tearDown" /> + <Bug pattern="DM_GC" /> + </Match> + <Match> + <!-- Test does not explicitly close statement deliberately --> + <Class name="org.apache.tomcat.jdbc.test.StatementFinalizerTest" /> + <Method name="testStatementFinalization" /> + <Bug pattern="ODR_OPEN_DATABASE_RESOURCE"/> + </Match> + <Match> + <!-- Choice of name is deliberate --> + <Class name="org.apache.tomcat.jdbc.test.TestException" /> + <Bug pattern="NM_CLASS_NOT_EXCEPTION" /> + </Match> + <Match> + <!-- Testing auto-close so connections not explicitly closed --> + <Class name="org.apache.tomcat.jdbc.test.TestGCClose" /> + <Or> + <Method name="testGCStop" /> + <Method name="testClose" /> + </Or> + <Bug pattern="ODR_OPEN_DATABASE_RESOURCE" /> + </Match> + <Match> <!-- SQL is from config so is considered safe --> <Class name="org.apache.tomcat.jdbc.test.TestSlowQueryReport" /> <Method name="testFastSql" /> @@ -1923,6 +1958,22 @@ <Bug pattern="SQL_PREPARED_STATEMENT_GENERATED_FROM_NONCONSTANT_STRING" /> </Match> <Match> + <!-- Tests throw exceptions so connections are never created --> + <Class name="org.apache.tomcat.jdbc.test.TestValidationQueryTimeout" /> + <Or> + <Method name="testValidationQueryTimeoutOnConnection" /> + <Method name="testValidationInvalidOnConnection" /> + <Method name="testValidationQueryTimeoutOnBorrow" /> + </Or> + <Bug pattern="ODR_OPEN_DATABASE_RESOURCE" /> + </Match> + <Match> + <!-- Statics used to work around API limitations --> + <Class name="org.apache.tomcat.jdbc.test.TestValidationQueryTimeout" /> + <Field name="isTimeoutSet" /> + <Bug pattern="ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD" /> + </Match> + <Match> <Class name="org.apache.tomcat.jdbc.test.TwoDataSources" /> <Method name="testTwoDataSources" /> <Or> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
