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

BVT updates #120

Merged
merged 14 commits into from
Jan 25, 2017
Merged

BVT updates #120

merged 14 commits into from
Jan 25, 2017

Conversation

AfsanehR-zz
Copy link
Contributor

No description provided.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense. will do

private static DBStatement stmt = null;

///////////////////////////////////////////////////////////////////
//// Connect to specified server and close the connection
Copy link
Contributor

Choose a reason for hiding this comment

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

can we change these comments into javadocs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya. will do.


terminateVariation();
}
}

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

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

DBSchema schema;
ArrayList<ArrayList<Object>> rows;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

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

Choose a reason for hiding this comment

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

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.


// Interleave resultset calls
rs1.next();
rs1.verifyCurrentRow(table1);
Copy link
Contributor

Choose a reason for hiding this comment

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

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)


rs.verify(table1);
stmt.close();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
}

}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

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

Choose a reason for hiding this comment

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

add javadocs for this method

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

Choose a reason for hiding this comment

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

can we remove the arg variable as its not used

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

break;

case java.sql.Types.VARBINARY:
assertTrue(parseByte((byte[])expectedData, (byte[])retrieved),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

*/
public void setObject(int parameterIndex, Object targetObject) throws SQLException {

((PreparedStatement) product()).setObject(parameterIndex, targetObject);
Copy link
Contributor

Choose a reason for hiding this comment

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

these javadocs needs to be updated

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
}

public static void terminateVariation() throws SQLException {
Copy link
Contributor

Choose a reason for hiding this comment

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

both the terminate method needs javadoc

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

Choose a reason for hiding this comment

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

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

@@ -51,6 +51,10 @@
super(dbConnection, null, "statement");
}

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

Choose a reason for hiding this comment

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

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

@@ -125,14 +125,14 @@ public String getEscapedTableName() {
*
* @return total rows in the table
*/
public int getTotalRows() {
public static int getTotalRows() {
return totalRows;
Copy link
Contributor

Choose a reason for hiding this comment

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

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


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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -39,6 +40,7 @@
protected Object maxvalue = null;
protected Object nullvalue = null; // Primitives have non-null defaults
protected VariableLengthType variableLengthType;
static Random r = new Random();
Copy link
Contributor

Choose a reason for hiding this comment

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

lets use ThreadLocalRandom instead

@AfsanehR-zz AfsanehR-zz merged commit 0faef74 into microsoft:dev Jan 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants