Skip to content
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

Odbc 172 update for connres and appveyor #814

Merged
merged 7 commits into from
Jul 13, 2018
Merged

Odbc 172 update for connres and appveyor #814

merged 7 commits into from
Jul 13, 2018

Conversation

david-puglielli
Copy link
Contributor

@david-puglielli david-puglielli commented Jul 11, 2018

… 17.2


This change is Reviewable

@coveralls
Copy link

coveralls commented Jul 11, 2018

Coverage Status

Coverage increased (+0.3%) to 74.958% when pulling cb78977 on david-puglielli:odbc-172-update into bbfe6df on Microsoft:dev.

@codecov-io
Copy link

codecov-io commented Jul 11, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##              dev     #814   +/-   ##
=======================================
  Coverage   80.06%   80.06%           
=======================================
  Files          25       25           
  Lines        7325     7325           
=======================================
  Hits         5865     5865           
  Misses       1460     1460

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 bbfe6df...cb78977. Read the comment docs.

Copy link
Contributor

@yitam yitam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For pdo ae tests, pls use skipif_mid-refactor.inc instead. Also, for your checks against 2k16 or 2k14, please check how isAEQualified() is implemented to include Azure DB.

Copy link
Contributor

@yitam yitam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider rewriting the 2k14 and 2k16 skipifs

@@ -7,7 +7,8 @@ killing the connection and then not freeing it. The latter case is the only one
that should fail. Finally, execute two queries in two threads on a recovered
non-MARS connection. This should fail too.
--SKIPIF--
<?php require('skipif_protocol_not_tcp.inc');
<?php require('skipif_mid-refactor.inc');
require('skipif_protocol_not_tcp.inc');
require('skipif_version_less_than_2k14.inc'); ?>
--FILE--
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe put skipif_mid-refactor.inc to the end? because it skips sql server < 2016?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skipif_mid-refactor doesn't skip any SQL Server unless AE is set, in which case it will skip versions < 2016. That's what we want isn't it?

@@ -1,7 +1,7 @@
--TEST--
Test the connection resiliency keywords ConnectRetryCount and ConnectRetryInterval and their ranges of acceptable values
--SKIPIF--
<?php require('skipif_unix.inc');
<?php require('skipif_mid-refactor.inc');
require('skipif_version_less_than_2k14.inc'); ?>
--FILE--
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment earlier about skipif_mid-refactor.inc

@@ -5,7 +5,8 @@ Prepare a statement, break the connection, and execute the statement. Then
test transactions by breaking the connection before beginning a transaction
and in the middle of the transaction. The latter case should fail.
--SKIPIF--
<?php require('skipif_protocol_not_tcp.inc');
<?php require('skipif_mid-refactor.inc');
require('skipif_protocol_not_tcp.inc');
require('skipif_version_less_than_2k14.inc'); ?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here... otherwise, this test will be skipped even when running against sql server 2014

@@ -4,7 +4,8 @@ Test connection resiliency timeouts
1. Connect with ConnectRetryCount equal to 0.
2. Reconnect with the default value of ConnectRetryCount(1).
--SKIPIF--
<?php require('skipif_protocol_not_tcp.inc');
<?php require('skipif_mid-refactor.inc');
require('skipif_protocol_not_tcp.inc');
require('skipif_version_less_than_2k14.inc'); ?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again this one too

@@ -1,6 +1,4 @@
<?php
if ( !( strtoupper( substr( php_uname( 's' ),0,3 ) ) === 'WIN' ) ) die( "Skip Test on windows only." );

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this one and the equivalent one for 2k14 need to check ODBC version? only works for 17.2 right?

@@ -2,6 +2,7 @@
Test the Authentication keyword and three options: SqlPassword, ActiveDirectoryIntegrated, and ActiveDirectoryPassword.
--SKIPIF--
<?php require('skipif.inc');
require('skipif_unix.phpt');
require('skipif_version_less_than_2k16.inc'); ?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think starting with ODBC 17 Azure AD works outside Windows too.

@david-puglielli
Copy link
Contributor Author

For some reason it's called $DriverName in MsSetup.inc instead of $driver. I'll fix it.

@yitam
Copy link
Contributor

yitam commented Jul 11, 2018

@david-puglielli $DriverName is only in pdo's MsSetup.inc, and I don't know what its use for.

@david-puglielli david-puglielli merged commit 9479d03 into microsoft:dev Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants