-
Notifications
You must be signed in to change notification settings - Fork 375
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
Refactor PDO tests with client buffer or table with all datatypes #582
Conversation
@@ -1,494 +0,0 @@ | |||
--TEST-- | |||
Test the different type of data for retrieving |
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 test was deleted because it is a duplication of pdostatement_GetDataType.phpt
Codecov Report
@@ Coverage Diff @@
## dev #582 +/- ##
==========================================
+ Coverage 74.89% 74.96% +0.07%
==========================================
Files 50 50
Lines 14915 14933 +18
==========================================
+ Hits 11171 11195 +24
+ Misses 3744 3738 -6
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.
I just started reviewing, and I'll continue later.
require_once("MsCommon_mid-refactor.inc"); | ||
if (isColEncrypted()) { | ||
$xmlType = "nvarchar(max)"; | ||
} else { |
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 probably should keep the xml
type with AE enabled
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.
why? xml is not supported for encrypted columns.
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.
yes, just thought we should test xml type even with AE enabled. There are hardly any other tests on XML type
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.
how do I test it if it's not supported? no dot encrypt the column explicitly?
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.
yes, just do not encrypt xml column or any other types that are not supported
$decimal_col = array(111.111, 222.222); | ||
|
||
$large_string = "This is a really large string used to test certain large data types like xml data type. The length of this string is greater than 256 to correctly test a large data type. This is currently used by atleast varchar type and by xml type. The fetch tests are the primary consumer of this string to validate that fetch on large types work fine. The length of this string as counted in terms of number of characters is 417."; | ||
|
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.
You mean at least
?
Even 417 is not that big...
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.
Now that I look at this the data is all simple chars... consider using more sophisticated data, at least with more extended ASCII characters?
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 the original data in MsData.inc. For this PR, I won't change it, but maybe we can add a task for testing with more sophisticated data on the PDO side.
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.
Sure please. No doubt this is way too simple.
"FloatCol" => "float", | ||
"XmlCol" => $xmlType); | ||
createTable($conn, $tbname, $columnMetaArr, "ON [PRIMARY]"); | ||
} catch (Exception $e) { |
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 test table is relatively small when compared to the test table in sqlsrv. Just saying ;)
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.
:P too bad. This is the original table schema
"NVarCharCol" => $string_col[$rownum], | ||
"FloatCol" => $decimal_col[$rownum], | ||
"XmlCol" => $xml_col[$rownum]); | ||
$stmt = insertRow($conn, $tbname, $inputs, "prepareBindParam"); |
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.
why do quite a few columns use the same data?
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.
again this is originally in MsData.inc
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.
Why don't you overwrite MsData.inc? Why creating a new include file?
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.
because we're in the middle of refactoring. Some not yet refactored tests may rely on the original MsData.inc
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.
Are there many of them left not refactored? Perhaps refactor them all for this PR?
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'm not sure. All the tests that use the old connect() function from MsCommon.inc could use the data in MsData.inc. This PR is already large enough I don't think it's a wise decision to refactor any more tests.
$textType = "text"; | ||
$xmlType = "xml"; | ||
} | ||
|
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.
Again, should probably keep the original data types even when AE is enabled?
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.
but these datatypes are not supported for encrypted columns
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 see my reasons for xml types
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'm half way through now. Will continue soon.
printf("Cannot find PDO driver line in phpinfo() output\n"); | ||
} else { |
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.
Perhaps rephrase the error message to refer to pdo_sqlsrv?
{ | ||
echo $e->getMessage(); | ||
if (($code != $expected) && (($expected != '00000') || ($code !=''))) { | ||
printf("[%03d] Expecting error code '%s' got code '%s'\n", $offset, $expected, $code); |
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.
What's the chance of $code being an empty string? Perhaps check it using empty()?
Also, in this test, looks like $expected is always '00000'? What's the point of having it as an argument?
Looks like the if condition is never true...
Lastly, please consider getting rid of unnecessary parentheses
printf("[%03d] Driver-specific error code not set\n", $offset); | ||
} | ||
if (!isset($info[2]) || ($info[2] == '')) | ||
{ | ||
if (!isset($info[2]) || ($info[2] == '')) { | ||
printf("[%03d] Driver-specific error message.not set\n", $offset); | ||
} |
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 remove the extra dot in the error message. Also, isset() probably suffices.
@$conn1->query("SELECT id, label FROM [$table2]"); | ||
checkError(12, $conn1, '42S02'); | ||
checkError(13, $conn2, '42S02'); | ||
checkError(14, $stmt1); |
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.
In the original test, quite a few lines have been commented. Do you know why?
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 actually don't know. Probably someone commented it out for debugging and just left it there.
//CheckError(7, $stmt2, '42S02'); | ||
dropTable($conn1, $table1); | ||
$stmt1->execute(); | ||
checkError(5, $conn1); |
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.
Where is the @ sign?
die ( print_r( sqlsrv_errors() ) ); | ||
$conn = new PDO("sqlsrv:server = $server ; Database = $dbName ;", $uid, $pwd); | ||
if ($conn === false) { | ||
die(print_r(sqlsrv_errors())); |
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.
Shouldn't exceptions be used instead of sqlsrv_errors()?
} | ||
|
||
// Create table | ||
$sql = "CREATE TABLE $tableName1 ( c1 INT, c2 VARCHAR(40) )"; | ||
$stmt = $conn->query( $sql ); | ||
$stmt = $conn->query($sql); | ||
|
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 remove the extra spaces in the sql statements
{ | ||
try { | ||
$conn = connect($connectionInfo, array(), PDO::ERRMODE_EXCEPTION, true); | ||
} catch (PDOException $e) { |
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.
perhaps explain why AE is disabled in this particular test?
// query with a wrong column name. | ||
$db->query( "Select * from " . $table1 . " where IntColX = 1" ); | ||
} | ||
$db->query("Select * from " . $tbname . " where IntColX = 1"); |
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.
Most tests use UPPER CASES for T-SQL keywords, so please change this as well.
unset($stmt); | ||
unset($conn); | ||
echo "Insert complete!\n"; | ||
} catch (PDOException $e) { | ||
var_dump($e); | ||
} |
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.
So this test only inserts and nothing else?
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.
yes, there is another test that inserts and selects
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.
are they very similar? If so just remove one and marked it as duplicate?
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.
Three tests are not shown as they were regarded as BINARY. Please double check why this happens. I'm done with the first round, finally
--FILE-- | ||
<?php | ||
require_once("MsCommon_mid-refactor.inc"); | ||
require_once("MsData_PDO_AllTypes.inc"); | ||
|
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.
Suggestion, include this MsData file in your MsCommon file instead. In case you need to change it later, you don't have to modify various tests that require this data file
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 pulled this out explicitly so in the future we know which tests use table with all datatypes. If I need to change the name of the file later I'll just do replace all :)
if(!$stmt->execute()) | ||
{ | ||
$stmt = $db->prepare("select * from $tbname"); | ||
if (!$stmt->execute()) { | ||
die("Test_1 failed."); | ||
} |
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.
use exception for this entire test please
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.
The whole test is wrapped by a try catch block. I think the test is OK now since it let us know which part of the test fails.
|
||
$tableName = "fruit"; | ||
//$tableName = "fruit"; | ||
$tableName = getTableName(); |
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.
Remove the commented line please
$options = array(PDO::ATTR_EMULATE_PREPARES => true); | ||
} | ||
$stmt = $conn->prepare($query, $options); | ||
$stmt->bindParam(':cal', $cal, PDO::PARAM_INT); |
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.
With AE enabled, perhaps no need to run this second block? It's essentially the same as the one above, isn't it?
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.
same as which one? pdo_prepare_emulatePrepare_float.phpt? This one is different, it binds int and uses PDO::PARAM_INT.
$stmt = $conn->query($query); | ||
if (!isColEncrypted()) { | ||
// without emulate prepare, binding PARAM_INT with SQLSRV_ENCODING_SYSTEM is not allowed | ||
// thus these are not tested when Column Encryption is disabled | ||
|
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 think you mean "the following will not be tested " when AE is enabled?
also, is not allowed or simply impossible, and not only with ENCODING_SYSTEM but the other two as well, I suppose? Please add a comment why.
There are several similar tests with emulate prepare, e.g. pdo_prepare_emulatePrepare_float.phpt
but you have refactored them differently. Please make them consistent.
regardless, shouldn't you handle such case with an error?
$stmt = $db->query("Select * from $tbname"); | ||
|
||
// Fetch the first column from the next row in resultset. (This wud be first row since this is a first call to fetchcol) | ||
$result = $stmt->fetchColumn(); |
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 use would, not wud :)
print_r($err); | ||
} | ||
} | ||
} | ||
|
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 refactor this test some more as many methods are very similar
@@ -1,76 +1,81 @@ | |||
--TEST-- | |||
Test the PDOStatement::getColumnMeta() method (Note: there could be an issue about using a non-existent column index --- doesn't give any error/output/warning). | |||
--SKIPIF-- | |||
<?php require('skipif.inc'); ?> | |||
<?php require('skipif_mid-refactor.inc'); ?> |
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.
Reduce the test and put more words into the description instead
@@ -1,124 +1,115 @@ | |||
--TEST-- | |||
Test the PDOStatement::getColumnMeta() method for Unicode column names (Note: there could be an issue about using a non-existent column index --- doesn't give any error/output/warning). | |||
--SKIPIF-- |
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.
The test name is too long. Use description instead.
} | ||
catch ( PDOException $e) | ||
{ | ||
} catch (PDOException $e) { |
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 refactor using loops and arrays to simplify the test
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.
Didn't review pdostatement_GetDataType.phpt because it's regarded as BINARY
{ | ||
$unsupportedTypes = array("money", "smallmoney", "image", "ntext", "text", "xml", "sql_variant"); | ||
if (in_array(strtolower($this->dataType), $unsupportedTypes) && !$this->forceEncrypt) { | ||
return false; |
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.
suggestion: test if forceEncrypt first?
foreach ( $dataTypes as $dataType ) { | ||
try { | ||
$conn = connect('', array(), PDO::ERRMODE_SILENT); | ||
foreach ($dataTypes as $dataType) { |
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.
Perhaps add a comment explaining why you set ERRMODE_SILENT
because not all the AE specific tests have this error mode
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.
The comment sounds unclear. Please rephrase when you have a chance.
echo "Test successfully done.\n"; | ||
DropTable( $conn, $tbname ); | ||
} | ||
DropTable($conn, $tbname); | ||
} |
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.
dropTable()
new columnMeta("nvarchar(50)", "LastName", "NULL"), | ||
new columnMeta("date", "BirthDate", null, "randomized")); | ||
createTable($conn, $tbname, $colMetaArr); | ||
|
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.
ColumnMeta
$conn1 = connect('', array(), PDO::ERRMODE_EXCEPTION, true); | ||
$stmt = $conn1->query($selectSql); | ||
while ($decrypted_row = $stmt->fetch(PDO::FETCH_ASSOC)) { | ||
if ($decrypted_row[ 'CharCount' ] == strlen($decrypted_row[getDefaultColName("nvarchar(1000)")])) { | ||
$rowInd = $decrypted_row[ 'CharCount' ] + 1; | ||
echo "Failed to encrypted at row $rowInd\n"; |
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 remove spaces within square brackets
$tbname = getTableName(); | ||
$colMetaArr = array( new columnMeta($dataType, "c_det"), new columnMeta($dataType, "c_rand", null, "randomized")); | ||
createTable($conn, $tbname, $colMetaArr); | ||
|
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.
ColumnMeta
@yitam , these ae tests were never tested on SQL Server 2K12 before, and seems to be more strict on decimal types. As for pdo_ae_insert_pdoparam_numeric.phpt, the initial expected values were wrong. |
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.
Just a couple more suggestions.
foreach ( $dataTypes as $dataType ) { | ||
try { | ||
$conn = connect('', array(), PDO::ERRMODE_SILENT); | ||
foreach ($dataTypes as $dataType) { |
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.
The comment sounds unclear. Please rephrase when you have a chance.
exit; | ||
} | ||
} | ||
|
||
function teardown() | ||
{ | ||
// TBD |
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.
There are quite a few methods in MsCommon.inc that are probably not very useful. Next time please consider merging your changes in mid-refactor to this one and removing the unused helper methods.
This change is