Junit testing by peterbae · Pull Request #404 · microsoft/mssql-jdbc

peterbae · 2017-07-21T17:18:08Z

Moved JDBCEncryptionDecryption AE tests into Junit. Ran Appveyor and Travis tests against my local dev branch to ensure that the tests passed.

msftclas · 2017-07-21T17:18:12Z

@peterbae,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

codecov-io · 2017-07-21T17:30:34Z

Codecov Report

Merging #404 into dev will increase coverage by 4.93%.
The diff coverage is 50%.

@@             Coverage Diff              @@
##                dev     #404      +/-   ##
============================================
+ Coverage     40.07%   45.01%   +4.93%     
- Complexity     1887     2132     +245     
============================================
  Files           107      107              
  Lines         24482    24661     +179     
  Branches       4038     4102      +64     
============================================
+ Hits           9812    11101    +1289     
+ Misses        12831    11693    -1138     
- Partials       1839     1867      +28

Flag	Coverage Δ	Complexity Δ
#JDBC41	`44.8% <50%> (+4.83%)`	`2118 <0> (+237)`	⬆️
#JDBC42	`44.85% <50%> (+4.93%)`	`2128 <0> (+250)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java	`59.61% <50%> (+12.09%)`	`90 <0> (+27)`	⬆️
...oft/sqlserver/jdbc/SQLServerParameterMetaData.java	`23.81% <0%> (-1.71%)`	`30% <0%> (ø)`
...m/microsoft/sqlserver/jdbc/KeyVaultCredential.java	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
...QLServerColumnEncryptionAzureKeyVaultProvider.java	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java	`49.85% <0%> (+0.42%)`	`214% <0%> (+3%)`	⬆️
...soft/sqlserver/jdbc/SQLServerDatabaseMetaData.java	`29.47% <0%> (+0.45%)`	`48% <0%> (+2%)`	⬆️
...n/java/com/microsoft/sqlserver/jdbc/DataTypes.java	`77.4% <0%> (+0.5%)`	`5% <0%> (ø)`	⬇️
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java	`59.57% <0%> (+0.5%)`	`138% <0%> (+9%)`	⬆️
...oft/sqlserver/jdbc/SQLServerResultSetMetaData.java	`40.84% <0%> (+0.7%)`	`23% <0%> (+1%)`	⬆️
.../microsoft/sqlserver/jdbc/SQLServerConnection.java	`45.78% <0%> (+0.71%)`	`286% <0%> (+19%)`	⬆️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f46f622...6b905af. Read the comment docs.

AfsanehR-zz · 2017-07-24T15:30:51Z

src/test/java/com/microsoft/sqlserver/jdbc/AlwaysEncrypted/AESetup.java

+    static final String cmkName = "JDBC_CMK";
+    static final String cekName = "JDBC_CEK";
+    static final String secretstrJks = "password";
+	static final String numericTable = "JDBCEncrpytedNumericTable";


can you run format on all of the file?

AfsanehR-zz · 2017-07-24T15:32:04Z

src/test/java/com/microsoft/sqlserver/jdbc/AlwaysEncrypted/AESetup.java

+     * @throws SQLException
+     */
+	static void dropTables() throws SQLException {
+		stmt.executeUpdate("if object_id('" + numericTable + "','U') is not null" + " drop table " + numericTable);


let's use Utils.dropTableIfExists

AfsanehR-zz · 2017-07-24T15:32:55Z

src/test/java/com/microsoft/sqlserver/jdbc/AlwaysEncrypted/JDBCEncryptionDecryptionTest.java

-     * 
-     * @throws Exception
-     * @throws TestAbortedException
+     * Junit test case for char set string for string values


same for this file. let's run format

AfsanehR-zz · 2017-07-24T15:35:42Z

src/test/java/com/microsoft/sqlserver/jdbc/AlwaysEncrypted/JDBCEncryptionDecryptionTest.java

+     * @throws SQLException
+     */
+    @Test
+	public void testChar_SpecificSetter() throws SQLException {


let's use camelCase convention, I noticed you used the right convention in the methods you introduced.

AfsanehR-zz · 2017-07-24T15:37:54Z

src/test/java/com/microsoft/sqlserver/jdbc/AlwaysEncrypted/JDBCEncryptionDecryptionTest.java

+		}
+
+		pstmt.execute();
+	}


missing pstmt.close in some places

I created Util.close(ResultSet, Statement, Connection) that employs good practice for closing those 3 objects, and applied it to the test cases.

AfsanehR-zz · 2017-07-24T15:42:23Z

src/test/java/com/microsoft/sqlserver/jdbc/AlwaysEncrypted/RandomData.java

@@ -0,0 +1,655 @@
+package com.microsoft.sqlserver.jdbc.AlwaysEncrypted;
+import java.math.BigDecimal;


I suggest moving this randomData class somewhere in test framework so we can use it in other tests as well. also, could you run form format?

yep. randomData and Util were originally for AE only, but I think they can be used elsewhere as well - i moved both of them to util package.

AfsanehR-zz · 2017-07-24T15:42:36Z

src/test/java/com/microsoft/sqlserver/jdbc/AlwaysEncrypted/RandomData.java

+	private static String numberCharSet = "1234567890";
+    private static String numberCharSet2 = "123456789"; 
+
+	//-------------------numeric types--------------------------------------------	


Change this // to javaDocs

AfsanehR-zz · 2017-07-24T15:44:04Z

src/test/java/com/microsoft/sqlserver/jdbc/AlwaysEncrypted/Util.java

+			return ((SQLServerConnection) connection).prepareCall(sql, ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY, connection.getHoldability(), stmtColEncSetting);
+		}
+	}
+	/*


you can remove these sections if they are not needed

AfsanehR-zz · 2017-07-24T15:45:08Z

src/test/java/com/microsoft/sqlserver/testframework/AbstractTest.java

        keyIDs = getConfiguredProperty("keyID", "").split(";");

-        connectionString = getConfiguredProperty("mssql_jdbc_test_connection_properties");
+        connectionString = getConfiguredProperty("mssql_jdbc_test_connection_properties")


add the + ";sendTimeAsDateTime=false"; inside the jdbcEncryptionDecryption test

done. I moved it to AESetup.java.

AfsanehR-zz · 2017-07-24T15:46:08Z

src/test/java/com/microsoft/sqlserver/jdbc/AlwaysEncrypted/AESetup.java

@@ -70,23 +78,24 @@ static void setUpConnection() throws TestAbortedException, Exception {

        readFromFile(javaKeyStoreInputFile, "Alias name");
        con = (SQLServerConnection) DriverManager.getConnection(connectionString);


add the + ";sendTimeAsDateTime=false"; here. We don't want other tests to create connection with having this connection property.

AfsanehR-zz · 2017-07-24T15:49:47Z

src/test/java/com/microsoft/sqlserver/jdbc/AlwaysEncrypted/AESetup.java

+	static final String binaryTable = "JDBCEncrpytedBinaryTable";
+	static final String dateTable = "JDBCEncrpytedDateTable";
+	static final String uid = "171fbe25-4331-4765-a838-b2e3eea3e7ea";
+	static final String uid2 = "171fbe25-4331-4765-a838-b2e3eea3e7eb";


is uid2 used anywhere?

Nope, I removed it.

AfsanehR-zz · 2017-07-24T15:52:23Z

src/test/java/com/microsoft/sqlserver/jdbc/AlwaysEncrypted/AESetup.java

        con = (SQLServerConnection) DriverManager.getConnection(connectionString);
-        stmt = con.createStatement();
+        stmt = (SQLServerStatement) con.createStatement();
        Utils.dropTableIfExists(numericTable, stmt);


since dropTables() method in afterAll() is cleaning up, let's remove this, or add all dropping tables here too.

AfsanehR-zz · 2017-07-24T16:04:33Z

src/main/java/com/microsoft/sqlserver/jdbc/Util.java

    // if driver is for JDBC 42 and jvm version is 8 or higher, then always return as SQLServerPreparedStatement42,
    // otherwise return SQLServerPreparedStatement
-    static boolean use42Wrapper() {
+    public static boolean use42Wrapper() {


Could you check using System.getProperty("java.specification.version"); and skipping the test if java 7 would solve the issue?

Checking for java 7 only doesn't solve the issue - the tests being done on github uses java 8, but does not support jdbc 4.2 - we need to check for both.

…cking and moved/added functionalities to utility classes.

…ality to the Util class within the testing package

peterbae added 5 commits July 19, 2017 14:23

Moved JDBCEncryptionDecryption AE tests into Junit

ce5e089

Adding one more file for AE Junit testing

f477068

Trying to solve setObject issue

85bb389

test

d6685c1

Made Util a public class to check for jdbc 4.2 compliance

5de4296

peterbae requested a review from AfsanehR-zz July 21, 2017 17:18

msftclas added the cla-required label Jul 21, 2017

ajlam added PR Under Review and removed cla-required labels Jul 21, 2017

AfsanehR-zz reviewed Jul 24, 2017

View reviewed changes

peterbae added 2 commits July 24, 2017 14:16

Employed Afsaneh's suggestions - did javadoc addition, did format che…

188d4d8

…cking and moved/added functionalities to utility classes.

Revert Util to be package private and add the jdbc 4.2 check function…

6b905af

…ality to the Util class within the testing package

AfsanehR-zz approved these changes Jul 25, 2017

View reviewed changes

peterbae merged commit c28b271 into microsoft:dev Jul 25, 2017

ajlam removed the PR Under Review label Jul 25, 2017

peterbae deleted the JunitTesting branch July 25, 2017 17:57

peterbae restored the JunitTesting branch July 25, 2017 17:57

		@@ -0,0 +1,655 @@
		package com.microsoft.sqlserver.jdbc.AlwaysEncrypted;
		import java.math.BigDecimal;

		@@ -70,23 +78,24 @@ static void setUpConnection() throws TestAbortedException, Exception {

		readFromFile(javaKeyStoreInputFile, "Alias name");
		con = (SQLServerConnection) DriverManager.getConnection(connectionString);

Conversation

peterbae commented Jul 21, 2017

Uh oh!

msftclas commented Jul 21, 2017

Uh oh!

codecov-io commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Jul 21, 2017 •

edited

Loading