BVT updates by AfsanehR-zz · Pull Request #120 · microsoft/mssql-jdbc

AfsanehR-zz · 2017-01-17T21:43:54Z

No description provided.

Suraiya-Hameed · 2017-01-17T23:17:08Z

+     * @param charSet
+     * @return
+     */
+    protected static String buildCharOrNChar(int columnLength, String charSet) {


move these new methods into SqlChar class, as this is an abstract class and we are not going to use these methods in all the classes that extends SqlType.

makes sense. will do

Suraiya-Hameed · 2017-01-18T21:50:22Z

+    private static DBStatement stmt = null;
+
+    ///////////////////////////////////////////////////////////////////
+    //// Connect to specified server and close the connection


can we change these comments into javadocs?

ya. will do.

Suraiya-Hameed · 2017-01-18T21:53:41Z

    ///////////////////////////////////////////////////////////////////
    // Create a statement with a query timeout
    // ResultSet.Type_forward_only,
    // ResultSet.CONCUR_READ_ONLY, executeQuery


update the comments as this test doesn't use ResultSet. Also review other comments too.

Suraiya-Hameed · 2017-01-18T22:07:51Z

+
+        try {
+            conn = new DBConnection(connectionString);
+            stmt = conn.createStatement(ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY);


Do you think it will be better to have an enum in DBStatement/DBConnection that has all combinations of resultSetType and resultSetConcurrency? So we don't have to call and set each type (will be useful to reuse in other tests), and just have validation in btvTest

Suraiya-Hameed · 2017-01-18T22:18:11Z

-    int totalRows = 2; // default row count set to 2
+    static int totalRows = 3; // default row count set to 3
    DBSchema schema;
+    ArrayList<ArrayList<Object>> rows;


any reason why we need rows and currentrow? wont the columns suffice?

rows is all rows we need to verify our backend data. verification is done row by row.

all the expected values must be available in
List<DBColumn> columns

try accessing them as
columns.get(Columnindex).getRowValue(row);

Suraiya-Hameed · 2017-01-18T22:39:31Z

+     * @throws SQLException
+     */
+    public void verifydata(int ordinal, Class coercion, Object arg) throws SQLException {
+        Object backendData = this.currentrow().get(ordinal);


The name backendData gives an impression that it is actual data from DB, when it is actually expected data. It would be better to rename this variable.

Suraiya-Hameed · 2017-01-18T22:44:16Z

+
+            // Interleave resultset calls
+            rs1.next();
+            rs1.verifyCurrentRow(table1);


currentTable variable in DBResultSet can be made protected, it will be less confusing. in line #429 currentTable is set and the same value is passed again in DBResultSet#verifyCurrentRow (line #437)

Suraiya-Hameed · 2017-01-18T22:48:07Z

+
+            rs.verify(table1);
+            stmt.close();
+        }


we should verify if the stmt/rs is closed here else this test will be same as the previous one.

Suraiya-Hameed · 2017-01-19T23:41:58Z

+        }
+    }
+
+}


add a method with @afterall tag to clean up the 2 tables.

Suraiya-Hameed · 2017-01-19T23:48:17Z

+    }
+
+    public void verifydata(int ordinal, Class coercion, Object expectedData, Object retrieved) throws SQLException {
+        metaData = this.getMetaData();


add javadocs for this method

Suraiya-Hameed · 2017-01-19T23:49:22Z

+     * @throws SQLException
+     */
+    public void verifydata(int ordinal, Class coercion, Object arg) throws SQLException {
+        Object expectedData = currentTable.columns.get(ordinal).getRowValue(_currentrow);


can we remove the arg variable as its not used

or if you have any future TODOs for this variable add it as comment

Suraiya-Hameed · 2017-01-19T23:55:50Z

+        if (coercion == Object.class) {
+            return this.getObject(idx);
+        }
+        return null;


can we have warning logged here if there is no valid coercion? It will be easier to trace if we try to test with unimplemented coercion

Suraiya-Hameed · 2017-01-20T00:04:09Z

+                break;
+
+            case java.sql.Types.VARBINARY:
+                assertTrue(parseByte((byte[])expectedData, (byte[])retrieved),


Can we have Arrays.equals() here instead of implementing parseByte()?

Suraiya-Hameed · 2017-01-20T00:05:42Z

+            DBResultSet.TYPE_SCROLL_INSENSITIVE, DBResultSet.CONCUR_READ_ONLY), TYPE_SCROLL_SENSITIVE_CONCUR_READ_ONLY(
+                    DBResultSet.TYPE_SCROLL_SENSITIVE, DBResultSet.CONCUR_READ_ONLY), TYPE_FORWARD_ONLY_CONCUR_UPDATABLE(
+                            DBResultSet.TYPE_FORWARD_ONLY, DBResultSet.CONCUR_UPDATABLE), TYPE_SCROLL_SENSITIVE_CONCUR_UPDATABLE(
+                                    DBResultSet.TYPE_SCROLL_SENSITIVE, DBResultSet.CONCUR_UPDATABLE), TYPE_DYNAMIC_CONCUR_OPTIMISTIC(


It will increase the readability if we don't use our formatter here!

Suraiya-Hameed · 2017-01-20T00:15:37Z

+     */
+    public void setObject(int parameterIndex, Object targetObject) throws SQLException {
+
+        ((PreparedStatement) product()).setObject(parameterIndex, targetObject);


these javadocs needs to be updated

Suraiya-Hameed · 2017-01-20T00:31:46Z

 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.jupiter.api.Assertions.fail;


can you also use jupiter.api.Assertions for assertEquals and assertTrue

Suraiya-Hameed · 2017-01-20T00:34:21Z

+                assertEquals(e.toString(), "com.microsoft.sqlserver.jdbc.SQLServerException: The result set is closed.");
+            }
+        }
+        finally {


it will be good if we can also have an assert after catch, to verify code execution dint reach that part of code.

the code will get executed after catch. we are just catching it and inside catch make sure that it has thrown the proper exception.

in case the exception is not thrown, the test will pass, we don't want that to happen.

Suraiya-Hameed · 2017-01-20T00:53:58Z

+        }
+    }
+
+    public static void terminateVariation() throws SQLException {


both the terminate method needs javadoc

Suraiya-Hameed · 2017-01-20T01:01:23Z

+     */
+    public DBPreparedStatement prepareStatement(String query) throws SQLException {
+        DBPreparedStatement dbpstmt = new DBPreparedStatement(this, internal, "preparedStatement");
+        return dbpstmt.prepareStatement(query);


the variable internal passed here must be preparedStatement object. Can we have DBPreparedStatement(DBConnection dbConnection) constructor in DBPreparedStatement like the one in DBStatement? it will reduce the confusion

Suraiya-Hameed · 2017-01-20T01:12:47Z

+    public static final int CONCUR_OPTIMISTIC = ResultSet.CONCUR_UPDATABLE + 2;
+    public static final int TYPE_CURSOR_FORWARDONLY = ResultSet.TYPE_FORWARD_ONLY + 1001;
+    public static final int TYPE_FORWARD_ONLY = ResultSet.TYPE_FORWARD_ONLY;
+    public static final int CONCUR_READ_ONLY = ResultSet.CONCUR_READ_ONLY;


can you add some documentation on why some values are added?

Suraiya-Hameed · 2017-01-20T01:19:39Z

+        DBResultSetMetaData wrapper = null;
+        try {
+            product = resultSet.getMetaData();
+            wrapper = new DBResultSetMetaData(parent, product, name);


update the constructor in DBResultSetMetaData to accept just DBResultSet, to be consistent with other classes that extend AbstractParentWrapper. Also, pass valid value for both parent and name variable, they are null at this point.

Suraiya-Hameed · 2017-01-20T01:26:47Z

+     */
+    public int getColumnType(int index) throws SQLException {
+        int current = ((SQLServerResultSetMetaData) product()).getColumnType(index);
+        return current;


can we directly return the value whenever possible rather than storing them in temp variable and then passing them? this way value will not be stored in memory while waiting for GC.

Suraiya-Hameed · 2017-01-20T17:59:40Z


+    DBStatement(DBConnection parent, Statement internal, int type, int concurrency, int holdability) {
+        super(parent, null, "statement");
+    }


this constructor can be removed as we are not using any of the new parameters passed

Suraiya-Hameed · 2017-01-20T18:03:27Z

     */
-    public int getTotalRows() {
+    public static int getTotalRows() {
        return totalRows;


why is totalRows as static? Shouldn't the number of rows be tied to a table!

Suraiya-Hameed · 2017-01-20T18:10:17Z

+
+        DateTimeOffset maxDTS = calculateDateTimeOffsetMinMax("max", precision, "9999-12-31 23:59:59");
+        DateTimeOffset minDTS = calculateDateTimeOffsetMinMax("min", precision, "0001-01-01 00:00:00");
+


replace the hard coded value with SqlTypeValue.DATETIMEOFFSET.maxValue/minValue

It will be better to move these calls into SqlDateTimeOffset() and store them in minvalue and maxvalue. So we don't have to calculate the same value over and over while generating random datetimeoffset value

Suraiya-Hameed · 2017-01-20T18:18:19Z

    protected Object maxvalue = null;
    protected Object nullvalue = null;	// Primitives have non-null defaults
    protected VariableLengthType variableLengthType;
+    static Random r = new Random();


lets use ThreadLocalRandom instead

AfsanehR-zz added 8 commits January 9, 2017 13:10

Merge branch 'dev' of https://github.com/Microsoft/mssql-jdbc into dev

638ec21

Merge branch 'dev' of https://github.com/Microsoft/mssql-jdbc into dev

bc94d2b

bvt updates

926ad00

bvt and testframework updates

9d0c483

bvt test updates

3ff9758

removed comment

ef04a4b

removed unused variables

f3a9b51

update to DBResultSet

315e8ae

Suraiya-Hameed suggested changes Jan 17, 2017

View reviewed changes

updates to sqlType and datetimeoffset creation

c8b4880

Suraiya-Hameed reviewed Jan 18, 2017

View reviewed changes

AfsanehR-zz added 3 commits January 19, 2017 11:09

adding dbresultsetTypes enum and

1498468

resolved conflicts and updated branch

6d06af3

added binary/varbinary verification

697d4eb

Suraiya-Hameed reviewed Jan 19, 2017

View reviewed changes

Suraiya-Hameed reviewed Jan 20, 2017

View reviewed changes

AfsanehR-zz added 2 commits January 20, 2017 16:52

post review commits

b7a85ce

updated datetimeoffset max,min

e2cc5a6

Suraiya-Hameed approved these changes Jan 24, 2017

View reviewed changes

AfsanehR-zz merged commit 0faef74 into microsoft:dev Jan 25, 2017

+                      }
+                  }
+              }


		DateTimeOffset maxDTS = calculateDateTimeOffsetMinMax("max", precision, "9999-12-31 23:59:59");
		DateTimeOffset minDTS = calculateDateTimeOffsetMinMax("min", precision, "0001-01-01 00:00:00");

Conversation

AfsanehR-zz commented Jan 17, 2017

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!

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!

3 participants