-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Testing of MSSQL on Windows with AppVeyor #2617
Conversation
Example build on AppVeyor https://ci.appveyor.com/project/photodude/dbal/build/1.0.17 /ping @deeky666 |
@deeky666 I'm not sure why the configuration xml files are failing to be read. I think that's all that needs to be sorted out (but we'll see once we can get tests running) |
@deeky666 Based on what I have seen in some forums there might be something wrong in the Bootstrap file configuration listed in the xml files as |
@photodude first of all thanks for the amazing effort! Much appreciated! I guess the error might be a Windows related issue, because the exact same configuration is working on Linux. You might be right and the current working directory might be evaluated differently in this setup. Maybe you could try to play around with the path to the Composer autoloader. Maybe something like |
@deeky666 According to another forum post I read maybe the bootstrap file is doing something like the following chdir(dirname(__FILE__) . "/../src/"); So that classes and autoload would find actual classes that are separated from the test classes. see http://stackoverflow.com/questions/21599640/phpunit-needs-an-absolute-path-to-phpunit-xml I do agree that it is some kind of windows specific configuration issue. |
One thought is we could copy the bootstrap file to this repo and make appveyor specific modifications rather than letting it live in the vendor directory. |
A note I found about the bootstrap file used in the XML located in another repo file 'vendor/autoload.php could not be found. Did you run `php composer.phar install`?' Seems this file is related to composer??? |
ok Looking at the directory the @deeky666 could this be a problem with the |
Yep the root issue is the Tests are now running https://ci.appveyor.com/project/photodude/dbal/build/1.0.21 Will review later to see if there are other configuration issues |
I'm not sure why but the builds are failing due to [Seld\JsonLint\ParsingException]
"./composer.lock" does not contain valid JSON
Parse error on line 1:
^
Expected one of: 'STRING', 'NUMBER', 'NULL', 'TRUE', 'FALSE', '{', '[' https://ci.appveyor.com/project/photodude/dbal/build/1.0.24/job/b3ur560vhh14xg62#L112 Seems like composer suddenly decided to be mad |
Maybe We should just kill the composer cache.... |
@deeky666 Looks like everything is running and passing now... minus the PGSQL tests which have one failure and some test is killing the overall unit testing progress (could use debug to find the last test which is stalling out) |
.gitattributes
Outdated
@@ -1,4 +1,3 @@ | |||
/tests export-ignore |
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 have to revert this, otherwise we cannot provide lightweight packages anymore. I think the problem is shallow_clone
in the CI config which seems to download the zip archive rather than really cloning the repository.
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've reverted the change and I'll fully remove shallow_clone to fix the underlying issue. Thanks for the tip.
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 has been implemented
.appveyor.yml
Outdated
- mssql2014 | ||
- mysql | ||
- postgresql94 | ||
- iis |
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.
Is this required for SQL Server?
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.
IIS is the webserver for windows (unless you install apache...something a bit more complicated)
.appveyor.yml
Outdated
- SET COMPOSER_NO_INTERACTION=1 | ||
- SET PHP=1 | ||
- SET ANSICON=121x90 (121x90) | ||
services: |
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.
Can we make the services load only if the current build environment requires them? Like "load mysql only if %db == 'mysql'".
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.
on travis we can put services in the matrix, I'm not sure if that is possible here.
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.
Maybe like this?
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 was just looking at that same item for these changes https://github.com/photodude/dbal/blob/patch-2/.appveyor.yml#L106-L113
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 has been implemented
.appveyor.yml
Outdated
|
||
## Install PHP and composer, and run the appropriate composer command | ||
install: | ||
- IF EXIST c:\tools\php (SET PHP=0) |
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 need this? PHP has to be installed on each build anyways, doesn't it? Can we get rid of the PHP
variable?
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 following a guide on the setup of php on appveyor. I've also been wondering If it's necessary or superfluous. https://blog.wyrihaximus.net/2016/11/running-php-unit-tests-on-windows-using-appveyor-and-chocolatey/
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.
Hmm weird it is not really explained there. Makes things quite complicated. We might leave it as is for now and improve later on.
tests/appveyor/mariadb.appveyor.xml
Outdated
@@ -0,0 +1,37 @@ | |||
<?xml version="1.0" encoding="utf-8"?> |
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 can remove this file, as we currently do not have MariaDB in the appveyor build matrix
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.
It's a placeholder, I'm looking into the possibility of installing MariaDB in appveyor as there is a windows version.
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've got MariaDB installed and running, but some adjustment to the DSN user and password are needed to get those tests to pass
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.
MariaDB is working now
tests/appveyor/mssql.appveyor.xml
Outdated
<php> | ||
<ini name="error_reporting" value="-1" /> | ||
|
||
<var name="db_type" value="pdo_sqlvr"/> |
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.
Can you change the names of the config files to the convention <dbal_driver_name>.appveyor.xml
so that we can have a test config for each driver? Then also please can you add another build for sqlsrv
driver?
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 followed and copied the Travis files for the config XMLs, is there a list I can use to make sure I have the right dbal driver names for the change?
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 believe we can also test on multiple MSSQL versions, if we want to wait for a longer list of builds to run. Also note, each version of MSSQL has a specific host name i.e. (local)\SQL2014
which is set in the xml files
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 can look up the driver names in the Driver classes like here.
A complete list of supported drivers currently is:
ibm_db2
mysqli
oci8
pdo_mysql
pdo_oracle
pdo_pgsql
pdo_sqlite
pdo_sqlsrv
sqlanywhere
sqlsrv
Simply leave those out that cannot (yet) be run on AppVeyor.
If different database versions and or vendors (MySQL/MariaDB) require different connection parameters, we should probably extend the convention to something like (<db_name>(.<db_version>)?.)?<dbal_driver_name>.appveyor.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.
I'll have to think about how to implement that suggested change in a way that could work with - vendor\bin\phpunit --verbose --debug -c tests\appveyor\%db%.appveyor.xml
and with If ($env:db -like "mysql")
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'd suggest that you define multiple environment variables in your build matrix like:
db
db_version
driver
Then you can do something like phpunit -c tests\appveyor\%db%.%db_version%.%driver%.appveyor.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.
Someone should also make the same kinds of changes to the Travis config XMLs
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 that could need some improvement, too.
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.
Considering the config XML files live in a CI service specific option can we drop the service name from the XML files?
so change tests/appveyor/<db_name>.<db_version>.<dbal_driver_name>.appveyor.xml
to tests/appveyor/<db_name>.<db_version>.<dbal_driver_name>.xml
?
tests/appveyor/mysql.appveyor.xml
Outdated
|
||
<testsuites> | ||
<testsuite name="Doctrine DBAL Test Suite"> | ||
<directory>C:\projects\dbal\tests\Doctrine\Tests\DBAL</directory> |
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 is an absolute path required here? Can we use ../Doctrine/Tests/DBAL
?
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.
that was a change related to figuring out why the config files could not be read. I'll revert the related changes.
tests/appveyor/mssql.appveyor.xml
Outdated
<php> | ||
<ini name="error_reporting" value="-1" /> | ||
|
||
<var name="db_type" value="pdo_sqlvr"/> |
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 can look up the driver names in the Driver classes like here.
A complete list of supported drivers currently is:
ibm_db2
mysqli
oci8
pdo_mysql
pdo_oracle
pdo_pgsql
pdo_sqlite
pdo_sqlsrv
sqlanywhere
sqlsrv
Simply leave those out that cannot (yet) be run on AppVeyor.
If different database versions and or vendors (MySQL/MariaDB) require different connection parameters, we should probably extend the convention to something like (<db_name>(.<db_version>)?.)?<dbal_driver_name>.appveyor.xml
.appveyor.yml
Outdated
- SET COMPOSER_NO_INTERACTION=1 | ||
- SET PHP=1 | ||
- SET ANSICON=121x90 (121x90) | ||
services: |
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.
Maybe like this?
.appveyor.yml
Outdated
|
||
## Install PHP and composer, and run the appropriate composer command | ||
install: | ||
- IF EXIST c:\tools\php (SET PHP=0) |
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.
Hmm weird it is not really explained there. Makes things quite complicated. We might leave it as is for now and improve later on.
tests/appveyor/sqlsrv.appveyor.xml
Outdated
<php> | ||
<ini name="error_reporting" value="-1" /> | ||
|
||
<var name="db_type" value="sqlvr"/> |
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.
typo: sqlsrv
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.
fixed
.appveyor.yml
Outdated
# - mssql2014 | ||
# - mysql | ||
# - postgresql94 | ||
- iis |
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 is a webserver required at all in our test setup? Is it required for SQL Server to run?
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'll try without it
.appveyor.yml
Outdated
echo extension=php_pdo_sqlsrv_nts.dll >> php.ini} | ||
If ($env:PHP -eq "1") { | ||
echo extension=php_sqlsrv_nts.dll >> php.ini}} | ||
- IF %PHP%==1 echo extension=php_pdo_pgsql.dll >> php.ini |
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.
Maybe if you work with DBAL driver names as environment variable you could even substitute it here and save some lines. Like: IF %PHP%==1 echo extension=php_%driver%.dll >> php.ini
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'll leave that for future improvement considerations. Let's get the core functionality in place and working.
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.
Ok. Agreed. :)
.appveyor.yml
Outdated
If ($env:db -like "mssql") { | ||
appveyor DownloadFile https://github.com/Microsoft/msphpsql/releases/download/4.1.5-Windows/7.0.zip | ||
7z x 7.0.zip > $null | ||
copy 7.0\x64\php_pdo_sqlsrv_7_nts.dll ext\php_pdo_sqlsrv_nts.dll |
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 would copy the DLLs to php_pdo_sqlsrv.dll
instead of php_pdo_sqlsrv_nts.dll
. This makes the naming consistent with the other driver's DLLs and you can potentially make use of variable substitution to save if...else
stuf
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 followed this convention since there is a TS (threadsafe) and a NTS (not threadsafe) versions for the drivers. chocolatey is currently installing the NTS version of PHP but are implementing a change that will allow forcing the TS version of PHP.
As for the condition, it is only in there to prevent copying the DLL line into the PHP.ini file when we don't have the DLLs downloaded (prevents warnings about being unable to load dll)
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 don't care about thread safety, that is nothing to worry about for us. So I don't see an issue with only considering the NTS driver. But I have no strong opinion about this. Just thought it could save some lines later when using variable substitution.
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.
might be worth considering rewriting this section with cinst wget -y
and wget the dll's as a faster option
https://www.microsoft.com/en-us/sql-server/developer-get-started/php-windows
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 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.
Looks like the DLLs will not register if they are not named with the NTS
Sadly wget was no faster and the PDO driver wouldn't always download.
.appveyor.yml
Outdated
- IF %PHP%==1 echo @php %%~dp0composer.phar %%* > composer.bat | ||
- appveyor-retry appveyor DownloadFile https://getcomposer.org/composer.phar | ||
- cd c:\projects\dbal | ||
- IF %dependencies%==lowest appveyor-retry composer update --prefer-lowest --no-progress --profile -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.
I still don't get this dependencies stuff from composer. Do we need it? You have set the environment variable to fixed current
anyways...
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.
Is there ever a time when testing might include lowest or highest dependencies? I set to current for the POC just for quicker testing
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 have never used it nor seen it at all. So I guess that kind of thing is not interesting for us. We do not have such thing in Travis either.
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.
If seen it in a few places where it's used in the "allowed failures" to get an idea of what's coming next and if the next release of a dependency breaks a package.
There are also times where it's used to allow for testing when the consuming repos might have higher or lower requirements and the package itself is typically happy in any case with a broader range of dependency versions (put they want to test for it when changes are made).
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 have removed the composer high/low dependencies stuff
@deeky666 after making the config XML file name changes I'm now seeing an issue where pdo_sqlsrv will fail as driver not found. https://ci.appveyor.com/project/photodude/dbal/build/1.0.50/job/cvckl39ntwh1gamo#L362 related xml line https://github.com/photodude/dbal/blob/patch-2/tests/appveyor/mssql.mssql2014.pdo_sqlsrv.appveyor.xml#L11 any thoughts? |
Looks like the |
.appveyor.yml
Outdated
driver: sqlsrv | ||
db_version: mssql2014 | ||
php_ver_target: 7.0 | ||
- db: sqlsrv |
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 needs to be mssql
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.
fixed
so the NTS part of the DLL is required, still trying to figure out why the PDO SQLSRV driver is not loading |
Just need to figure out why we are getting the following errors in the build PDOException: SQLSTATE[28000]: [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Login failed for user 'sa'. and https://ci.appveyor.com/project/photodude/dbal/build/1.0.87/job/6j0lu02x4ao27i6s#L304 SQLSrvException: SQLSTATE [28000, 18456]: [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Login failed for user 'sa'.
SQLSTATE [42000, 4060]: [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Cannot open database "doctrine_tests" requested by the login. The login failed.
SQLSTATE [28000, 18456]: [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Login failed for user 'sa'.
SQLSTATE [42000, 4060]: [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Cannot open database "doctrine_tests" requested by the login. The login failed. following the docs the DSN info should be correct here |
Looking at the build times I suggest we consider just testing the MSSQL drivers on AppVeyor (unless the Doctrine Project wants to consider paying for additional concurrent jobs so parallel testing could be implimented) |
@photodude looks great so far. Concerning the build failures, I have two clues:
Can you try to change this line to return |
I'll try that change, if it's a bug see bc8a6df |
@deeky666 Looks like changing that line to I've got some suggested help from AppVeyor's @IlyaFinkelshteyn who said the databases are not being created. The suggestion is to check the part our script that is responsible for database creation. see support topic http://help.appveyor.com/discussions/problems/6010-appveyoryml-scripting-issues#comment_41862813 So we possibly have database creation issues, maybe the Sharding or Sharding tests for SQL are bugged as well |
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.
LGTM once squashed (i.e. one commit with AppVeyor changes + test fixes from @morozov, please drop my empty commit).
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.
AppVeyor green
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.
Looks like caching of Chocolatey and Composer packages could use some improvement, but it can be done later.
👍
This is awesome! @photodude thanks a lot for pushing this forward: it was a heck of a marathon! |
@morozov Note: While rebasing @photodude Can you please link some issue about the 7.2 so we can keep an eye on it? I want to port this to ORM too and it requires 7.2 already. |
Thanks everyone for all the help getting this complete and merged in. Shout out to @deeky666, @morozov, @Majkl578 who got the tests into the green and made all the suggestions for getting this into a state to merge. Without their help this would still be a work in progress. |
@photodude thank you for not giving this up! It's an incredible achievement for the project. |
@Majkl578 you'll want to watch this issue appveyor/ci#2155 |
Thanks @morozov It's nice to see this implemented so there is better testing with more supported platforms included in the automatic tests. |
great job @photodude thank you! |
- ps: >- | ||
if ($env:db -eq "mssql") { | ||
$instanceName = $env:db_version.ToUpper() | ||
Start-Service "MSSQL`$$instanceName" |
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.
@photodude is there a reason we use Start-Service
and not net start
? As per the Appveyor's suggestion in appveyor/ci#2204 (comment), it could be the reason for intermittent login failures.
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.
It's based on appveyor documentation https://www.appveyor.com/docs/services-databases/
If they have some new thing they suggest to do, their documentation does not reflect that.
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.
@photodude thanks. Trying to change this in #3071.
This is an initial draft [POC] for testing of MSSQL on windows with AppVeyor
This PR creates the initial draft configuration files for AppVeyor CI testing, adjustments may be needed to meet project configuration needs.
For more information see issue #2616