-
Notifications
You must be signed in to change notification settings - Fork 118
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
run tests with MySQL, PostgreSQL, SQLite #421
Conversation
888aaf5
to
306f9f7
Compare
PostgreSQL is fine now, at least with PHP > 7.3. I'm gonna remove support for PHP 7.3 but not within this PR. Awaiting feedback from @franmomu. 😊 |
29eb888
to
00e4ed1
Compare
Alright, now I also got all PHPUnit steps running even in case of 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.
LGTM!
- name: run PHPUnit with MySQL | ||
if: always() | ||
run: vendor/bin/phpunit -v --coverage-clover build/logs/clover-mysql.xml --group run-with-multiple-databases,run-with-multiple-databases-only | ||
env: | ||
DB_FLAVOR: mysql | ||
|
||
- name: run PHPUnit with PostgreSQL | ||
if: always() | ||
run: vendor/bin/phpunit -v --coverage-clover build/logs/clover-postgresql.xml --group run-with-multiple-databases,run-with-multiple-databases-only | ||
env: | ||
DB_FLAVOR: postgresql | ||
|
||
- name: run PHPUnit with SQLite | ||
if: always() | ||
run: vendor/bin/phpunit -v --coverage-clover build/logs/clover-sqlite.xml --group run-with-multiple-databases,run-with-multiple-databases-only | ||
env: | ||
DB_FLAVOR: sqlite |
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 can be moved to its own job I guess, at the same level as tests
and creating a new one tests-with-databases
, that way maybe it's clearer (I guess these tests don't need to be executed using different versions of Symfony).
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.
But then I'd have to decide with which PHP/Symfony versions they'd be run with. As of now, cross coverage is a lot better. Seeing tests fail only with PostgreSQL and PHP 7.3 is worth doing so.
/** | ||
* @var Connection | ||
*/ | ||
private $conn; | ||
|
||
protected function getConnection() : Connection { | ||
return $this->conn; | ||
} | ||
|
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 this is not used anymore
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 thought to just leave it there in case it'll be needed. 😁
But you're right. If it's really needed, it could be added again easily. Removed.
00e4ed1
to
1fae5f6
Compare
Different approach compared to #420. This makes it possible to run specific tests (only those which actually use a database) against several database platforms. I definitely like that the mechanism is easier to understand and tests themselves are kept clean.
What I don't like is that a failed step causes following steps to be skipped so if tests fail with MySQL they won't be run with PostgreSQL and SQLite. Also the
SYMFONY_DEPRECATIONS_HELPER
will be harder to use with 4 PHPUnit steps.I wonder why
PhotoUploadFlowTest
fails with PostgreSQL. 🤔 This at least proves that testing against several database platforms is a good idea. 😂