-
Notifications
You must be signed in to change notification settings - Fork 425
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
Statement test cases #147
Statement test cases #147
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #147 +/- ##
============================================
+ Coverage 27.32% 29.63% +2.31%
- Complexity 1142 1247 +105
============================================
Files 95 97 +2
Lines 23279 23303 +24
Branches 3870 3871 +1
============================================
+ Hits 6360 6906 +546
+ Misses 15641 15046 -595
- Partials 1278 1351 +73
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still reviewing.
import com.microsoft.sqlserver.testframework.DBConnection; | ||
import com.microsoft.sqlserver.testframework.DBStatement; | ||
|
||
@RunWith(JUnitPlatform.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to have class & method level javadocs for all classes in this PR
@Test | ||
@DisplayName("Named Param Multi Part Test") | ||
public void datatypestest() throws Exception { | ||
Connection Connection1 = DriverManager.getConnection(connectionString); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need connection1 & statment1? Because in AbstractTest we open one connection and accessible for all child classes. If somehow this connection fails due to may be wrong db connectionstring etc then all testcases which extends AbstractTest get skipped due to failure of setup method.
For specific testing it is goo to create your own connection & close that one but this case for connection1 we are not doing anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. This was part of the original test. Will remove it.
public void poolableTest() throws SQLException, ClassNotFoundException { | ||
Class.forName("com.microsoft.sqlserver.jdbc.SQLServerDriver"); | ||
Connection Connection1 = DriverManager.getConnection(connectionString); | ||
Statement Statement151 = Connection1.createStatement(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Statement151. :) I think stmt might be more appropriate.
@RunWith(JUnitPlatform.class) | ||
public class CallableMixed extends AbstractTest { | ||
Connection connection1 = null; | ||
Statement Statement151 = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I found non standard naming conventions. Instance Variables should start with lowercase. Generally identified as camelCase / lowerCamelCase. Look difference between connection1 and Stmatement151 / Driver1. Please change all instance variables with appropriate camelCase naming convention.
PS: Generally only static / final variables are in CAPS otherwise camelCase is pretty standard.
|
||
@Test | ||
@DisplayName("Test CallableMix") | ||
public void datatypestest() throws SQLException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function / Method name should be in lowerCamelCase / camelCase. This name should be dataTypesTest
.
*/ | ||
@Test | ||
@DisplayName("Test CallableMix") | ||
public void datatypestest() throws SQLException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately GitHub Reviews are not getting reflected after code file changes. :(
Here method name is not following lowerCamelCase nomenclature. It should be dataTypesTest
//TODO: Check the intermittent verification error | ||
//verifydata(ordinal, coercion, expectedData, retrieved); | ||
// TODO: Check the intermittent verification error | ||
// verifydata(ordinal, coercion, expectedData, retrieved); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really want to verify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not related to this pr. we need to investigate this later for the intermittent bvt failures
return isSqlAzure; | ||
} | ||
|
||
public double serverversion() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can give better function name like getServerVersion()
} | ||
|
||
// Returns the default server version, not to be used in multiple server versions case | ||
public static double serverversion() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw another serverversion()
in DBConnection
with different implementation. Which is right? Can we move this function in AbstractTest ?
public class WrapperTest extends AbstractTest { | ||
|
||
@Test | ||
public void WrapTest() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function name should be lowerCamelCase like
wrapTest
public static final String SERVER_TYPE_SQL_SERVER = "SQL"; | ||
public static final String SERVER_TYPE_SQL_AZURE = "SQLAzure"; | ||
|
||
public static String servertype() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Formatting is not consistent for servertype method.
- Method name should be like getServerType(...) for better naming convention.
- Do you think this might be utility method and can be used by many test cases?
return _serverType; | ||
} | ||
|
||
public static String get(String property){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really no need of this method. Instead of this method you can call Utils.getConfiguredProperty
@DisplayName("Test CallableMix") | ||
public void datatypestest() throws SQLException { | ||
connection1 = DriverManager.getConnection(connectionString); | ||
Statement151 = connection1.createStatement(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All instance variable's name should be in lowerCamelCase while static final variable names should be in CAPS.
isPoolable = ((SQLServerPreparedStatement) prepStmt).isPoolable(); | ||
assertEquals(isPoolable, true, "SQLServerPreparedStatement should be Poolable by default"); | ||
|
||
|
||
CallableStatement CallableStatement1 = Connection1.prepareCall("{ ? = CALL " + "ProcName" + " (?, ?, ?, ?) }"); | ||
CallableStatement CallableStatement1 = connection.prepareCall("{ ? = CALL " + "ProcName" + " (?, ?, ?, ?) }"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CallableStatement1 should be callableStatement1. lowerCamelCase.
} | ||
|
||
public double serverversion() throws Exception { | ||
public double getServerversion() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change function name in lowerCamelCase i.e. getServerVersion
// default to SQL Server | ||
_serverType = SERVER_TYPE_SQL_SERVER; | ||
} | ||
else if (serverTypeProperty.equalsIgnoreCase(SERVER_TYPE_SQL_AZURE)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like to have constant on left hand side for comparison. Although it's good that you are checking serverTypeProperty is null nor not before any comparison.
added unit test cases