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

WIP - Introduce tests on platforms #549

Closed
wants to merge 4 commits into from

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Feb 17, 2024

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.

@VincentLanglet VincentLanglet force-pushed the test-platform branch 5 times, most recently from fa32670 to 8778452 Compare February 17, 2024 18:39
* @group platform
* @group mysql
*/
class MysqlQueryResultTypeWalkerTest extends PDOQueryResultTypeWalkerTest
Copy link
Contributor Author

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

.github/workflows/test-platforms.yml Show resolved Hide resolved
.github/workflows/test-platforms.yml Show resolved Hide resolved
fail-fast: false
matrix:
php-version:
- "8.2"
Copy link
Contributor

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;
Copy link
Contributor

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) {
Copy link
Contributor

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

Copy link
Contributor Author

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.

@janedbal
Copy link
Contributor

The matrix (at some level) should also contain:

  • driver (there are more for mysql and pgsql and some of them behave differently)
  • config (mainly stringification on/off for PDO drivers)

@VincentLanglet
Copy link
Contributor Author

The matrix (at some level) should also contain:

  • config (mainly stringification on/off for PDO drivers)

Since the config must be passed to the entity manager in a entity-manager.php passed to phpstan in config with

parameters:
	doctrine:
		objectManagerLoader: entity-manager.php

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...

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.

None yet

2 participants