Refactored SQLSRV tests to test AE with various data types #575

yitam · 2017-10-24T00:06:55Z

Mainly the tests using a test table with 28 data types

This change is

coveralls · 2017-10-24T00:20:33Z

Coverage increased (+0.02%) to 73.221% when pulling c55ad3e on yitam:Refactor2 into b59fa30 on Microsoft:dev.

codecov-io · 2017-10-24T16:22:52Z

Codecov Report

Merging #575 into dev will increase coverage by 2.88%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev     #575      +/-   ##
==========================================
+ Coverage   72.01%   74.89%   +2.88%     
==========================================
  Files         125       50      -75     
  Lines       38990    14915   -24075     
==========================================
- Hits        28079    11171   -16908     
+ Misses      10911     3744    -7167

Impacted Files	Coverage Δ
...php-7.0.25-src/ext/pdo_sqlsrv/shared/core_stmt.cpp	`66.18% <0%> (-1.24%)`	⬇️
...x86/php-7.0.25-src/ext/sqlsrv/shared/core_stmt.cpp	`78.87% <0%> (-1.16%)`	⬇️
...x86/php-7.0.25-src/ext/sqlsrv/shared/core_conn.cpp	`70.18% <0%> (-0.22%)`	⬇️
...x86/php-7.1.10-src/ext/sqlsrv/shared/core_stmt.cpp
...x64/php-7.0.25-src/ext/sqlsrv/shared/core_conn.cpp
...x86/php-7.0.24-src/ext/sqlsrv/shared/core_stmt.cpp
...php-7.0.25-src/ext/pdo_sqlsrv/shared/core_sqlsrv.h
...php-7.0.24-src/ext/pdo_sqlsrv/shared/core_init.cpp
...x86/php-7.1.10-src/ext/sqlsrv/shared/core_util.cpp
...6/php-7.1.10-src/ext/sqlsrv/shared/core_stream.cpp
... and 120 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b59fa30...4ac4245. Read the comment docs.

coveralls · 2017-10-24T16:32:10Z

Coverage increased (+0.02%) to 73.221% when pulling 63d7a99 on yitam:Refactor2 into b59fa30 on Microsoft:dev.

yukiwongky · 2017-10-25T17:14:02Z

test/functional/sqlsrv/MsCommon.inc

 function createProc($conn, $procName, $procArgs, $procCode)
 {
    dropProc($conn, $procName);
    $stmt = sqlsrv_query($conn, "CREATE PROC [$procName] ($procArgs) AS BEGIN $procCode END");
    if ($stmt === false) {
-        fatalError("Failed to create test procedure");
+        fatalError("Failed to create test procedure", true);


default is already true, no need to put this in

yukiwongky · 2017-10-25T17:14:13Z

test/functional/sqlsrv/MsCommon.inc

@@ -509,7 +376,7 @@ function callProcEx($conn, $procName, $procPrefix, $procArgs, $procValues)
 {
    $stmt = sqlsrv_query($conn, "{ $procPrefix CALL [$procName] ($procArgs)}", $procValues);
    if ($stmt === false) {
-        fatalError("Failed to call test procedure");
+        fatalError("Failed to call test procedure", true);


same as line 355

yukiwongky · 2017-10-25T18:11:44Z

test/functional/sqlsrv/MsHelper.inc

+            !strcasecmp($this->dataType, "hierarchyid") ||
+            !strcasecmp($this->dataType, "geography") ||
+            !strcasecmp($this->dataType, "geometry") ||
+            !strcasecmp($this->dataType, "alias")) {


money and smallmoney are also not supported, why comment out?

also it would look nicer to put all the unsupported types in an array and there an array_in function that you can use in php to find a match

when we do not encrypt money or smallmoney one of the AE tests with money types fail...
that's why I changed the ColumnMeta to take an optional parameter to not to encrypt a particular column instead

yukiwongky · 2017-10-25T18:21:08Z

test/functional/sqlsrv/MsHelper.inc

        $this->dataType = $dataType;
        $this->options = $options;
+
+        // first asssumes the column is not encryptable
+        $this->encryptable = false;


why not put this in an else block instead of assuming?

yukiwongky · 2017-10-25T18:21:48Z

test/functional/sqlsrv/MsHelper.inc

+     */
+    protected function checkIfUnsupported()
+    {
+        $this->encryptable = true;


same here, you can put this into an else block

yukiwongky · 2017-10-25T18:25:20Z

test/functional/sqlsrv/MsHelper.inc

@@ -325,6 +377,7 @@ function connect($options = array(), $disableCE = false)
 * @param  object  $conn : sqlsrv connection object
 * @param  string  $tbname : name of the table to be created
 * @param  array  $columnMetaArr : array of ColumnMeta objects, which contain metadata for one column
+ * @return object sqlsrv statement


I believe sqlsrv statement is a resource not an object, same with sqlsrv connection

yukiwongky · 2017-10-25T18:29:14Z

test/functional/sqlsrv/MsHelper.inc

-                $r = sqlsrv_execute($stmt);
+                if ($stmt) {
+                    $r = sqlsrv_execute($stmt);
+                }


if you're going to check $stmt, maybe output the error as well

yukiwongky · 2017-10-25T18:45:33Z

test/functional/sqlsrv/MsHelper.inc

@@ -370,32 +425,106 @@ function insertRow($conn, $tbname, $inputs, &$r = null, $api = INSERT_QUERY)
        $params = array();
        foreach ($inputs as $key => $input) {
            if (is_object($input)) {
-                array_push($params, $input->bindParamArr($inputs[$key]));               
+                array_push($params, $input->bindParamArr($input->value));


this is very strange looking. If you're calling a member function from $input, the function will have access to everything in $input. Passing in $input->value is not necessary. Please change bindParamArr function accordingly.

I reverted the change now. Thanks!

yukiwongky · 2017-10-25T20:18:43Z

test/functional/sqlsrv/MsHelper.inc

+ */
+function executeQuery($conn, $sql, $conds = null, $values = null)
+{
+    // when AE is enabled, use sqlsrv_prepare() to handle fields with unlimited size


how can you tell if a field has unlimited size from here? If AE is enabled, does it always use sqlsrv_prepare?

yes, I'll modify the comments accordingly

yukiwongky · 2017-10-25T20:26:44Z

test/functional/sqlsrv/MsHelper.inc

+ * all deterministic if AE is enabled
+ * @param  resource  $conn : connection resource
+ * @param  string  $tbname : name of the table to create
+ * @return object sqlsrv statement


sqlsrv statement is a resoure

yukiwongky · 2017-10-25T20:34:38Z

test/functional/sqlsrv/MsHelper.inc

+ * @param  resource  $conn : connection resource
+ * @param  string  $tbname : name of the table to create
+ * @param  int  $index : the index of a certain row of test data
+ * @return object sqlsrv statement upon success or false otherwise


yukiwongky · 2017-10-25T20:50:29Z

test/functional/sqlsrv/MsHelper.inc

+
+    $result = null;
+    if (isColEncrypted()) {
+        $stmt = insertRow($conn, $tbname, $inputArray, $result, INSERT_PREPARE_PARAMS);


INSERT_PREPARE_PARAMS is redundant since in insertRow, isColEncrypted() and INSERT_PREPARE_PARAMS go to the same path (line 435)

yukiwongky · 2017-10-25T20:51:20Z

test/functional/sqlsrv/MsHelper.inc

 *                          literal or a BindParamOption object
 * @param  bool  $r : true if the row was successfully inserted, otherwise false. Default value is null to make this parameter optional.
 * $param  string  $api : SQLSRV API used for executing the insert query
- *                      accepted values: INSERT_QUERY, INSERT_PREPARE, INSERT_QUERY_PARAMS, INSERT_PREPARE_PARAMS
+ *                        accepted values: INSERT_QUERY, INSERT_PREPARE, INSERT_QUERY_PARAMS, INSERT_PREPARE_PARAMS


since you've already defined these constants, they are no longer string, use long

I changed it to int

yukiwongky · 2017-10-25T20:54:51Z

test/functional/sqlsrv/MsHelper.inc

+                // this might be an input to a decimal, a numeric or a binary field
+                if (isBinary($col)) {
+                    $value = "0x" . $value;    // annotate the input string as a hex string
+                }


can't you call insertRow after you've modified the binary data?

no I can't because binary data needs special handling which the generic function insertRow not supposed to know

yukiwongky · 2017-10-25T21:02:19Z

test/functional/sqlsrv/MsHelper.inc

+    // get array of input values
+    $inputArray = getInsertArray($rowIndex);
+    if (empty($inputArray)) {
+        fatalError("getInsertData: failed to retrieve data at row $rowIndex and column $colIndex.\n");


get rid of "and column $colIndex" since the error comes from only $rowIndex

yukiwongky · 2017-10-25T21:03:27Z

test/functional/sqlsrv/MsHelper.inc

@@ -449,7 +773,7 @@ function getSqlType($k)
    return ("udt");
 }

-function getDriverType($k, $dataSize)
+function getDriverType($k, $dataSize = 512)


where does 512 come from?

It was for convenience, but I only need it for one test, so I'll move this constant to that test instead.

yukiwongky · 2017-10-25T21:07:16Z

test/functional/sqlsrv/TC24_Close.phpt

-    insertRows($conn1, $tableName, $noRows);
+    // Insert a couple of random rows
+    AE\createTestTable($conn1, $tableName);
+    AE\insertTestRows($conn1, $tableName, 2);


why change $noRows from 5 to 2?

Just thought the essence of the test is not really the data. Anyway, I've reverted the change.

yukiwongky · 2017-10-25T21:08:32Z

test/functional/sqlsrv/TC24_Close.phpt

-    $conn2 = connect();
-    $stmt2 = selectFromTable($conn2, $tableName);
+    $conn2 = AE\connect();
+    $stmt2 = sqlsrv_query($conn2, "SELECT * FROM [$tableName]");


why can't you call selectFromTable?

yukiwongky · 2017-10-25T21:11:47Z

test/functional/sqlsrv/TC31_SimpleQuery.phpt


    trace("Executing SELECT query on $tableName ...");
-    $stmt1 = selectFromTable($conn1, $tableName);
+    $stmt1 = sqlsrv_query($conn1, "SELECT * FROM $tableName");


why not use createTable and selectFromTable?

yukiwongky · 2017-10-25T21:12:02Z

test/functional/sqlsrv/TC31_SimpleQuery.phpt


-    createTable($conn1, $tableName);
+    // just create an empty table
+    $stmt1 = sqlsrv_query($conn1, "CREATE TABLE $tableName (dummyColumn int)");

    trace("Executing SELECT query on $tableName ...");


I thought we were going to get rid of trace

I don't mind keeping them around. Some tests have useful trace statements.

But we never turn tracemode on.. If you think the output is useful, then use echo or print

I personally do when I need to debug what goes wrong. If I use echo/print if will affect the expected output, which is often unnecessary.

yukiwongky · 2017-10-25T21:13:19Z

test/functional/sqlsrv/TC31_SimpleQuery.phpt

    $rows = rowCount($stmt1);
-    ;
    sqlsrv_free_stmt($stmt1);


please dropTable at the end of the test

yukiwongky · 2017-10-25T21:26:35Z

test/functional/sqlsrv/TC32_DeleteQuery.phpt

-            $cond = "(c1_int = $keyValue)";
-
-            $stmt3 = selectFromTableEx($conn1, $tableName, $cond);
+            $cond = "(c1_int = ?)";


Is it a good idea to change the test to use a placeholder completely? Now even if isColEnc() is false, the test still uses sqlsrv_prepare with placeholder to run the query, whereas before is used to use sqlsrv_query on a direct query. When refactoring tests, we want to keep the behavior of the original test as intact as possible.

I decided to put the logic in AE\executeQuery() instead so that each placeholder will be replaced with literal value if not encrypted

yukiwongky · 2017-10-25T23:21:55Z

test/functional/sqlsrv/TC38_InvalidQuery.phpt

-    createTableEx($conn1, $tableName, $dataType);
-
+    $columns = array(new AE\ColumnMeta('int', 'c1'), 
+                     new AE\ColumnMeta('int', 'c2'));


where is createTable?

yukiwongky · 2017-10-25T23:41:40Z

test/functional/sqlsrv/TC43_FetchData.phpt

-            } else { // should check data even if $fld is null
-                $data = getInsertData($startRow + $i, $col, $skipCount);
-                if (!CheckData($col, $fld, $data)) {
+            $data = AE\getInsertData($startRow + $i, $col);


how come you getInsertData before checking if $col isUpdatable?

Can you also explain what $skipCount does and why you got rid of it?

Only the timestamp column (column 27) is updatable, and the original way to look for insert data is to skip this column. I'll explain more if this is unclear.

OK I'm beginning to understand what $skipCount is for now. But I still don't understand why you have getInsertData in line 47. There is an identical line in line 50, and $data is only used inside the isUpdatable if block.

Yes, it was an oversight. Thanks!

yukiwongky · 2017-10-25T23:44:54Z

test/functional/sqlsrv/TC46_FetchNextResult.phpt

-    if (sqlsrv_next_result($stmt1) ||
-        sqlsrv_next_result($stmt2)) {
+    if ($r1 = sqlsrv_next_result($stmt1) ||
+        $r2 = sqlsrv_next_result($stmt2)) {


why do you need to introduce $r1 and $r2?

It was for debugging when I thought I discovered a bug... forgot to remove them afterwards ;)

yukiwongky · 2017-10-25T23:47:05Z

test/functional/sqlsrv/TC47_ParamQuery.phpt

+            AE\insertRow($conn1,
+                $tableName,
+                array("c1"=>$k, "c2"=>array($data, SQLSRV_PARAM_IN, SQLSRV_PHPTYPE_STRING(SQLSRV_ENC_CHAR), $phpDriverType)), 
+                $res,


can you just put null here?

I just tried to be faithful to the original test, in which it called a function insertRowEx() that does the following:
$stmt = sqlsrv_query($conn, "INSERT INTO [$tableName] ($dataCols) VALUES ($dataValues)", $dataOptions);

?? I was talking about $res

nope... I will get the error Fatal error: Uncaught Error: Cannot pass parameter 4 by reference

yukiwongky · 2017-10-26T00:06:46Z

test/functional/sqlsrv/TC52_StreamSend.phpt


    sqlsrv_close($conn1);

    endTest($testName);
 }

-function CreateQueryTable($conn, $table, $dataType, $dataCols, $dataValues, $dataOptions, $execMode)
+function insertQueryTable($conn, $tableName, $params, $execMode)


can you not use insertRow for this function?

no because it's specific to this test, in which they set the option SendStreamParamsAtExec on and off for testing

yukiwongky · 2017-10-26T00:17:30Z

test/functional/sqlsrv/TC64_TransactionProc.phpt

                break;
        }
        if ($data != null) {
-            $sqlType = GetSqlType($k);
+            $sqlType = getSqlType($k);
+            $driverType = getDriverType($k);


the name of this function is very misleading. I thought getDriverType gets the PHP zend type... can you change the name of this to getSqlsrvSqltype() or something more clear?

I agree that it's confusing, but that was one of the common functions originally in MsCommon... I'll see how many tests are affected by the renaming.

yukiwongky · 2017-10-26T00:25:29Z

test/functional/sqlsrv/TC74_ProcQuery.phpt

@@ -74,40 +74,50 @@ function ProcQuery($minType, $maxType)
    endTest($testName);
 }

-function ExecProcQuery($conn, $procName, $dataType, $inData1, $inData2, $outData)
+function execprocQuery($conn, $procName, $type, $dataType, $inData1, $inData2, $outData)


execProcQuery

yukiwongky · 2017-10-26T00:26:14Z

test/functional/sqlsrv/TC75_ProcRoundtrip.phpt

 {
-    include 'MsSetup.inc';
+    // include 'MsSetup.inc';


delete this line

yukiwongky · 2017-10-26T00:27:21Z

test/functional/sqlsrv/TC79_RegressionVSTS846501.phpt

-    $s = sqlsrv_query($conn1, "DROP TABLE test_bq");
-    $s = sqlsrv_query($conn1, "CREATE TABLE test_bq (id INT IDENTITY NOT NULL, test_varchar_max varchar(max))");
+    $tableName = "test_bq";
+    dropTable($conn1, $tableName);


I think createTable already drop the table before creating the table, so this line is not necessary.

coveralls · 2017-10-26T18:00:15Z

Coverage increased (+0.01%) to 73.21% when pulling 4d364e1 on yitam:Refactor2 into b59fa30 on Microsoft:dev.

coveralls · 2017-10-26T19:56:06Z

Coverage increased (+0.02%) to 73.221% when pulling 786bd50 on yitam:Refactor2 into b59fa30 on Microsoft:dev.

yukiwongky · 2017-10-26T22:33:10Z

test/functional/sqlsrv/MsHelper.inc

        $this->dataType = $dataType;
        $this->options = $options;
+
+        // first asssumes the column is not encryptable
+        // $this->encryptable = false;


Please get rid of comments

yukiwongky · 2017-10-26T22:57:14Z

test/functional/sqlsrv/MsHelper.inc

-    // when AE is enabled, use sqlsrv_prepare() to handle fields with unlimited size
-    if (!isColEncrypted() && (is_null($conds) || empty($values))) {
+    if (!isColEncrypted()) {
+        if (!empty($values)) {


can you add a comment saying this if block is for replacing '?' placeholders with values

coveralls · 2017-10-27T17:10:46Z

Coverage decreased (-0.2%) to 73.043% when pulling 83a378b on yitam:Refactor2 into b59fa30 on Microsoft:dev.

coveralls · 2017-10-31T18:11:43Z

Coverage decreased (-0.2%) to 73.043% when pulling 4ac4245 on yitam:Refactor2 into b59fa30 on Microsoft:dev.

Refactored SQLSRV tests to test AE with various data types

c55ad3e

Print error messages for fatal errors

63d7a99

Changed huge bigint values to strings

2bcaf17

yitam requested a review from yukiwongky October 24, 2017 23:18

yukiwongky reviewed Oct 25, 2017

View reviewed changes

yukiwongky reviewed Oct 26, 2017

View reviewed changes

Modified tests as per review comments

4d364e1

Fixed tests with money or binary types

786bd50

yukiwongky reviewed Oct 26, 2017

View reviewed changes

yitam added 3 commits October 27, 2017 09:08

Modified tests based on review

886b0af

Use the correct skipif

886a7d7

Missed a few tests for skipif

83a378b

Added insertRowEx function

4ac4245

yukiwongky approved these changes Oct 31, 2017

View reviewed changes

yitam merged commit bd001a5 into microsoft:dev Oct 31, 2017

yitam deleted the Refactor2 branch October 31, 2017 22:00

Refactored SQLSRV tests to test AE with various data types #575

Refactored SQLSRV tests to test AE with various data types #575

Conversation

yitam commented Oct 24, 2017 • edited by lilgreenbird Loading

coveralls commented Oct 24, 2017

codecov-io commented Oct 24, 2017 • edited Loading

Codecov Report

coveralls commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yitam Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yukiwongky Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 26, 2017

coveralls commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 27, 2017

coveralls commented Oct 31, 2017

yitam commented Oct 24, 2017 •

edited by lilgreenbird

Loading

codecov-io commented Oct 24, 2017 •

edited

Loading

yitam Oct 25, 2017 •

edited

Loading

yukiwongky Oct 25, 2017 •

edited

Loading