Split akv verification tests to fix SSL issue #781

david-puglielli · 2018-05-25T00:22:58Z

Removed *_verification.phpt and replaced with _usernamepassword.phpt and _clientsecret.phpt.

This change is

codecov-io · 2018-05-25T00:44:40Z

Codecov Report

Merging #781 into dev will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev     #781   +/-   ##
=======================================
  Coverage   80.06%   80.06%           
=======================================
  Files          25       25           
  Lines        7323     7323           
=======================================
  Hits         5863     5863           
  Misses       1460     1460

Impacted Files	Coverage Δ
.../php-7.1.17-src/ext/sqlsrv/shared/core_results.cpp
...phpdev/vc14/x86/php-7.1.17-src/ext/sqlsrv/conn.cpp
...php-7.1.17-src/ext/pdo_sqlsrv/shared/core_util.cpp
...phpdev/vc14/x86/php-7.1.17-src/ext/sqlsrv/init.cpp
...ev/vc14/x86/php-7.1.17-src/ext/sqlsrv/php_sqlsrv.h
...vc14/x86/php-7.1.17-src/ext/pdo_sqlsrv/pdo_dbh.cpp
...php-7.1.17-src/ext/pdo_sqlsrv/shared/core_conn.cpp
...6/php-7.1.17-src/ext/sqlsrv/shared/core_stream.cpp
...4/x86/php-7.1.17-src/ext/pdo_sqlsrv/pdo_parser.cpp
...x86/php-7.1.17-src/ext/sqlsrv/shared/core_conn.cpp
... and 40 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 f0eefaf...ff0e460. Read the comment docs.

yitam

Several things: (1) please drop test tables at the end of your tests (2) now that the tests are split, they share the same code except how to connect -- suggestion: for easier maintenance, move the same methods to one file to be included in the tests (3) why str size 64? If it's a constant, you might consider using the keyword const and upper case letters for the constants (4) to be consistent with the naming scheme, please add underscores _ for the file names usernamepassword and clientsecret (5) please fix the comments in values.php

david-puglielli · 2018-05-25T20:15:55Z

Regarding strsize, it's 64 just to make sure the column size is large enough to accommodate the string values in values.php that are used for this test. But it changes in other AE tests that use other longer strings in values.php.

yitam

Please read my comments / questions.

yitam · 2018-05-25T20:50:21Z

test/functional/pdo_sqlsrv/pdo_ae_azure_key_vault_common.php

+// Set up the columns and build the insert query. Each data type has an
+// AE-encrypted and a non-encrypted column side by side in the table.
+// If column encryption is not set in MsSetup.inc, this function simply
+// creates two non-encrypted columns side-by-side for each type.


Not sure if I agree with your comment here because you have a specific connection string in your test, so it's not really affected by the flags in MsSetup?

ColumnMeta does use the flags in MsSetup though, so setting $keystore and $dataEncrypted to encrypt in MsSetup will encrypt the _AE columns if the rest of the connection string enables encryption.

yitam · 2018-05-25T20:54:09Z

test/functional/pdo_sqlsrv/pdo_ae_azure_key_vault_keywords.phpt

-require_once("MsCommon_mid-refactor.inc");
-require_once("MsSetup.inc");
-require_once('values.php');
+require_once('pdo_ae_azure_key_vault_common.php');



Do you drop the your test table(s) in this test and the sqlsrv equivalent one?

Yes, at the end of insertDataAndVerify() in the new common files.

hmmm... I don't see this insertDataAndVerify() anywhere... not sure how I miss it?

It's in sqlsrv_ae_azure_key_vault_common.php and pdo_ae_azure_key_vault_common.php.

yitam · 2018-05-25T20:54:36Z

test/functional/pdo_sqlsrv/pdo_ae_azure_key_vault_username_password.phpt

+require_once('pdo_ae_azure_key_vault_common.php');
+
+$strsize = 64;
+


Please add a comment to explain why you picked 64 for the strsize

yitam · 2018-05-25T20:55:27Z

test/functional/sqlsrv/sqlsrv_ae_azure_key_vault_client_secret.phpt

+require_once('sqlsrv_ae_azure_key_vault_common.php');
+
+$strsize = 64;
+


Add comment about 64 please

yitam · 2018-05-25T20:56:31Z

test/functional/sqlsrv/sqlsrv_ae_azure_key_vault_keywords.phpt

@@ -114,7 +85,7 @@ for ($i = 0; $i < sizeof($columnEncryption); ++$i) {
                    checkErrors(
                        $errors,
                        array('08001','0'),
-                        array('08001','-1'),    // SSL error occurs in Ubuntu
+                        array('08001','-1'),    // SSL error on some Linuxes
                        array('IMSSP','-110'),


Do we still have SSL errors after this split?

Yes, because I did not split the keywords test. This test runs through 254 connections so splitting is not practical.

How often does the SSL error occur? Does it always happen after the first successful connection?

I don't remember how often it happens...

yitam · 2018-05-25T20:57:26Z

test/functional/sqlsrv/sqlsrv_ae_azure_key_vault_username_password.phpt

+require_once('sqlsrv_ae_azure_key_vault_common.php');
+
+$strsize = 64;
+


you use 64 in almost every AKV test -- why not create a constant in values.php and reference it in all your tests?

That's a good idea...

yitam

Let's not forget to fix the remaining SSL issues
Drop tables in pdo keyword test too
Otherwise approved.

coveralls · 2018-05-26T00:15:52Z

Coverage remained the same at 74.697% when pulling ff0e460 on david-puglielli:akv-verification-test-fix into f0eefaf on Microsoft:dev.

Split akv verification tests to fix SSL issue

cee6370

david-puglielli requested a review from yitam May 25, 2018 00:22

yitam reviewed May 25, 2018

View reviewed changes

Refactored some akv tests, other small changes

bd9d85b

yitam reviewed May 25, 2018

View reviewed changes

david-puglielli added 2 commits May 25, 2018 14:42

addressed review comments

1747df1

dropped tables at end

ff0e460

yitam approved these changes May 25, 2018

View reviewed changes

david-puglielli merged commit 0861f06 into microsoft:dev May 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split akv verification tests to fix SSL issue #781

Split akv verification tests to fix SSL issue #781

david-puglielli commented May 25, 2018 •

edited by lilgreenbird

Loading

codecov-io commented May 25, 2018 •

edited

Loading

yitam left a comment

david-puglielli commented May 25, 2018

yitam left a comment

yitam May 25, 2018

david-puglielli May 25, 2018

yitam May 25, 2018

david-puglielli May 25, 2018

yitam May 25, 2018

david-puglielli May 25, 2018

yitam May 25, 2018

yitam May 25, 2018

yitam May 25, 2018

david-puglielli May 25, 2018

yitam May 25, 2018

david-puglielli May 25, 2018

yitam May 25, 2018

david-puglielli May 25, 2018

yitam left a comment

coveralls commented May 26, 2018 •

edited

Loading

		require_once('pdo_ae_azure_key_vault_common.php');

		$strsize = 64;

		require_once('sqlsrv_ae_azure_key_vault_common.php');

		$strsize = 64;

Split akv verification tests to fix SSL issue #781

Split akv verification tests to fix SSL issue #781

Conversation

david-puglielli commented May 25, 2018 • edited by lilgreenbird Loading

codecov-io commented May 25, 2018 • edited Loading

Codecov Report

yitam left a comment

Choose a reason for hiding this comment

david-puglielli commented May 25, 2018

yitam left a 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

yitam left a comment

Choose a reason for hiding this comment

coveralls commented May 26, 2018 • edited Loading

david-puglielli commented May 25, 2018 •

edited by lilgreenbird

Loading

codecov-io commented May 25, 2018 •

edited

Loading

coveralls commented May 26, 2018 •

edited

Loading