Fix for #35 and new integration test #304
Conversation
|
ping @alextech |
There was a problem hiding this comment.
I made a PR request to suggest fixes for comments. (I tested it against travis in another repository while messing with forks https://travis-ci.org/alextech/zend-db-integration)
I also do not think separating engines for high level utility tests, such as TableGateway, is good. This again creates false positive passes when someone modifies TableGateway, writes test for it in MySQL and does not replicate the test to other platforms. This is of course fine to do for low level Adapter tests, but high-level packages that are engine abstractions, same test must run and pass on all engines automatically. If same abstraction test fails and needs engine-specific handling, then abstraction is badly designed at that point. So current adapter should be injected as dependency into testSetup (or whichever way), and each one will be tested using separate travis configuration blocks.
I can help to think how to do this and update my PR in a little while.
What is the plan for existing tests? Is it to migrate tests for SQL generation here over time? It will be huge effort, but number of "hypothetical" queries in the tests provides scary amount of false positives. I admit I am equally guilty of testing against invalid SQL queries, just to check general function flow. If yes, will this be merged in to provide infrastructure and avoid fork juggling to work on that?
.travis.yml
Outdated
| - TESTS_ZEND_DB_ADAPTER_DRIVER_PGSQL_HOSTNAME=localhost | ||
|
|
||
| services: | ||
| - mysql |
There was a problem hiding this comment.
Move services to individual configuration blocks.
This will allow:
- Ability to configure specific multiple supported versions. Eg, PgSQL 10 is not BC with 9.6.
- Prevent travis from running multiple conflicting engines/configurations per build. Harder to see what version failed, unknown consequences.
.travis.yml
Outdated
| @@ -53,6 +59,8 @@ matrix: | |||
| - php: 7.2 | |||
| env: | |||
| - DEPS=locked | |||
| - TEST_INTEGRATION=true | |||
| - TESTS_ZEND_DB_ADAPTER_DRIVER_MYSQL=true | |||
There was a problem hiding this comment.
Put service here. One block per DB.
| } | ||
| } | ||
|
|
||
| private function createMysqlDatabase($dbFile) |
There was a problem hiding this comment.
Keep to convention used around the codebase of having this in Platform class. Will make possible adding other engines.
| getenv('TESTS_ZEND_DB_ADAPTER_DRIVER_MYSQL_DATABASE') | ||
| ))) { | ||
| throw new Exception(sprintf( | ||
| "I cannot create the MySQL %s test database", |
There was a problem hiding this comment.
In exceptions, add display of actual error occurred with $this->pdo->errorInfo().
print_r($this->pdo->errorInfo(), true) gives minimum formatting.
I had this fail and couldn't tell what was wrong.
|
|
||
| if (false === $this->pdo->exec(file_get_contents($dbFile))) { | ||
| throw new Exception(sprintf( | ||
| "I cannot create the table for %s database. Check the %s file. ", |
There was a problem hiding this comment.
As above in this exception.
But one for "Use" may not be needed, as i could not find ways it could fail for MySQL.
test/integration/TestAsset/mysql.sql
Outdated
| @@ -0,0 +1,21 @@ | |||
| CREATE TABLE IF NOT EXISTS test ( | |||
There was a problem hiding this comment.
Rename file and path to "fixtures" to use more widely used terminology.
|
@alextech I think we should have specific integration tests for all the db engine that we support. Even for special cases like #35, where we need to reproduce issues like #288 and #178 (comment).
If you change
Again, this is actually very good, because we can discover issue and bad design. Tests are useful to find mistakes, they don't prove that everything is ok (it's never ok as you know, even if you have 100% test coverage). Anyway, I'm reviewing your comment and I'm glad that you improved the PR with ezimuel#1, thanks! |
Feature/integration tests
Exactly. So I did not mean that unit tests should be used instead of integration. I meant that integration should not be in an adapter specific namespace for Design Pattern implementations. Design Pattern implementations, unlike direct adapter interaction have added responsibility of having same API work simultaneously on all engines. When you write TableGateway test in MySQL namespace that ends up not working for some reason in SqlServer, PostgreSQL as the documentation advertises, its a problem. As random example, https://github.com/zendframework/zend-db/pull/304/files#diff-56dd6fd011c00c8a8c735d14eed5c841R27 If that test passes for MySQL but not PostgreSQL, is that not a false positive? Is everyone going to be diligent to copy, https://github.com/zendframework/zend-db/pull/304/files#diff-56dd6fd011c00c8a8c735d14eed5c841R88 4 times to our 4 supported platforms to verify something as basic as $tableGateway->update() works as advertised? That does not mean don't keep them as integration. I mean it should not be isolated to one namespace or manually copied to all namespaces, but live in top level integration namespace and reran automatically for each integration engine in travis yaml config. |
|
@alextech ok, now I see your point. I agree to move the |
|
Yes, there are some integration tests for TableGateway that are engine specific, eg, using delimited path as a table name for Pgsql and SqlServer. I just remembered that case, sorry! So yes to
|
This PR fixes issue #35 and proposes a new integration test suite for
zend-db(using the travis-ci services,mysqlat the moment).The issue #35 has been solved removing the internal mapping of the field name with PDO parameter name, using a
c_$iincremental naming convention.The new test folder contains unit test (
test/unit) and integration test (test/integration) in separate folders. The test databases for integration are created using aZendIntegrationTest\Db\IntegrationTetListenerfile. This new approach simplify the travis-ci integration without the usage of the*_fixtures.shfiles in.cifolder.