-
Notifications
You must be signed in to change notification settings - Fork 95
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
WIP - Introduce tests on platforms #549
Conversation
fa32670
to
8778452
Compare
* @group platform | ||
* @group mysql | ||
*/ | ||
class MysqlQueryResultTypeWalkerTest extends PDOQueryResultTypeWalkerTest |
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.
Currently I extends PDOQueryResultTypeWalkerTest but later we'll work his own dataprovider
8778452
to
4498953
Compare
fail-fast: false | ||
matrix: | ||
php-version: | ||
- "8.2" |
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 definitelly need to check "before 8.1" and "after 8.1" as there were some major changes.
$one = new One(); | ||
$one->id = (string) $id++; | ||
$one->intColumn = $intColumn; | ||
$one->stringColumn = $stringColumn; |
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 definitelly need entities suited for platform testing (having float, decimal, bool, int, bigint columns).
$em->persist($one); | ||
} | ||
|
||
foreach (self::combinations($dataJoinedInheritance) as $combination) { |
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 not sure we need this complexity. I ended with super simple test fetching a single expression and verifying that type in https://github.com/janedbal/php-database-drivers-fetch-test All we need here is to check that against the inferred PHPStan type.
And I feel this is the best approach to accomplish what we need as it can be quite readable in the test itself
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 just copied the existing QueryResultTypeWalkerTest
with the whole dataprovider.
Do you think we should split it ?
- Only single expression for platforms tests
- single expression and complex expression for PDO tests in order to validate PHPStan for these
I thought this was better to keep the same data for every platform.
The matrix (at some level) should also contain:
|
Since the config must be passed to the entity manager in a
I don't see how it would be in the matrix. Currently, the only solution I see is to create one entityManager for every platform/config and one test case for every entityManager... |
Related to #506 (comment) @janedbal,
I try to introduce tests on different platform. Does this solution would work for you ?
Edit: Are you also ok with this solution @ondrejmirtes ?
I had in mind to provide
@group platform
on every platform tests, each with a special group like@group sql
.And adding all different platform in github actions.