Updates to test framework#90
Conversation
|
|
||
| //TODO: create a driver class to getConnection | ||
| DBConnection con = new DBConnection(); | ||
| con.getConnection(connectionString); |
There was a problem hiding this comment.
connection.getConnection() returns nothing makes me feel unfamiliar in the first glance. what do you think if we make the DBConnection() constructor accept connectionString and create actual connection?
There was a problem hiding this comment.
yes, that's sounds better, will update it.
| DBStatement stmt = con.createStatement(); | ||
| DBTable sourceTable = new DBTable(true); | ||
| sourceTable.createTable(stmt); | ||
| sourceTable.populateTable(stmt); |
There was a problem hiding this comment.
instead of DBTable.createTable(DBStatement) or DBTable.populateTable(DBStatement), can we do DBStatement.createTable(DBTable) and DBStatement.populateTable(DBTable) ? to be consistent with actual JDBC code.
| srcResultSet.close(); | ||
| validateValues(con, sourceTable, destinationTable); | ||
| sourceTable.dropTable(stmt); | ||
| destinationTable.dropTable(stmt); |
There was a problem hiding this comment.
Also here, can we do DBStatement.dropTable(DBTable) ? to be consistent with actual JDBC code.
can we also move dropTable() methods into a finally block to make sure table is dropped when test fails.
|
|
||
| case java.sql.Types.CHAR: | ||
| case java.sql.Types.NCHAR: | ||
| assertTrue((((String) srcValue).equals(((String) dstValue))), "Unexpected char/nchar value "); |
There was a problem hiding this comment.
why we have .trim() for varchar and nvarchar but not for char or nchar?
There was a problem hiding this comment.
for char and nchar the trailing space is preserved, so I din't add it. Now that you mention it, do you think they needed for nvarchar and varchar?
There was a problem hiding this comment.
To be even I'll remove trim() form both char and varchar
| break; | ||
|
|
||
| default: | ||
| System.out.println("Unknow type "+destMeta.getColumnType(i)); |
There was a problem hiding this comment.
should we throw exception if there is a missing type?
| /** | ||
| * Container for all the columns | ||
| */ | ||
| class DBColumns { |
There was a problem hiding this comment.
this class is called DBColumns. can we merge it with DBTable? I feel they should be identical.
There was a problem hiding this comment.
sounds fair, I'll update it.
| } catch (ClassNotFoundException e) { | ||
| e.printStackTrace(); | ||
| } catch (SQLException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
just wondering why we need to catch those 2 exceptions? can't we just throw the exceptions?
There was a problem hiding this comment.
It's best practice not to throw generic Exception whenever possible, so I've added individual ones (since its just 2).
| public void close() throws SQLException | ||
| { | ||
| if(null != resultSet) | ||
| resultSet.close(); |
There was a problem hiding this comment.
I remember we decided to use {} for single line if statement
3 participants
No description provided.