Skip to content

Allow to test hydration mode in platform DB. #587

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

Closed
wants to merge 2 commits into from

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Jun 26, 2024

WDYT @janedbal ? is it a good way to introduce test about different hydration mode ?

@VincentLanglet VincentLanglet marked this pull request as ready for review June 26, 2024 13:22
@janedbal
Copy link
Contributor

I was thinking about just using all hydratation modes that should be equal in performDriverTest. Just fetch it multiple ways and assert those match.

@VincentLanglet
Copy link
Contributor Author

I was thinking about just using all hydratation modes that should be equal in performDriverTest. Just fetch it multiple ways and assert those match.

But hydration mode doesn't always give the same result like bigInt which is a numeric-string with hydrate_object but an int with hydrate_scalar.
Also, depending on having hydrate_scalar, hydrate_single_scalar, hydrate_scalar_colum, the result is an array of array, just a scalar or an array of scalar.

@janedbal
Copy link
Contributor

Since 95% of the dataset in platform test uses DQL expressions, those should behave equally in all hydratation modes (I think, didnt check). Then there are few cases with entity field fetches (+ TypedExpressions) which would differ. So maybe we can keep your MR and assert those assumptions:

  • no mode given (null) = expects ALL modes to return the same
  • specific mode given = expects the given mode to return the given expected value

@VincentLanglet
Copy link
Contributor Author

Since 95% of the dataset in platform test uses DQL expressions, those should behave equally in all hydratation modes (I think, didnt check).

For instance, SELECT o.bigIntColumn from ... will return

  • An array of array of one string with HYDRATE_OBJECT
  • An array of array of one string with HYDRATE_ARRAY
  • An array of array of one int with HYDRATE_SCALAR
  • An int with HYDRATE_SINGLE_SCALAR
  • An array of int with HYDRATE_SCALAR_COLUM

So for the same query,

  • sometimes the bigInt is represented as string, sometimes as int ; based on the hydrationMode.
  • sometimes it's an array, sometimes an array of array and sometimes a simple scalar.

The second point need extra logic, I currently don't have in the PR.
But the first point...

Then there are few cases with entity field fetches (+ TypedExpressions) which would differ. So maybe we can keep your MR and assert those assumptions:

  • no mode given (null) = expects ALL modes to return the same
  • specific mode given = expects the given mode to return the given expected value

Yes, this could be a nice solution.

So I updated the PR to use null by default to go in this direction. b3b2f99

But it still require to add extra logic to check ALL modes return the same. And so far this dev does not seems to be an easy one to me, specially because I didn't succeed running the docker locally...

@janedbal
Copy link
Contributor

Specially because I didn't succeed running the docker locally...

How is that, which part of the readme is wrong?

@VincentLanglet
Copy link
Contributor Author

Specially because I didn't succeed running the docker locally...

How is that, which part of the readme is wrong?

When I run

docker-compose -f tests/Platform/docker/docker-compose.yml up -d

I get

#0 69.09 E: Package 'msodbcsql17' has no installation candidate
------
failed to solve: process "/bin/sh -c apt update      && apt install -y gnupg2     && apt install -y unixodbc-dev unixodbc     && curl https://packages.microsoft.com/keys/microsoft.asc | apt-key add -     && curl https://packages.microsoft.com/config/debian/11/prod.list > /etc/apt/sources.list.d/mssql-release.list     && apt update     && ACCEPT_EULA=Y apt install -y msodbcsql17     && pecl install sqlsrv-5.11.1     && pecl install pdo_sqlsrv-5.11.1     && docker-php-ext-enable sqlsrv pdo_sqlsrv" did not complete successfully: exit code: 100

and didn't found time to debug this ATM

@janedbal
Copy link
Contributor

I checked this a bit and I think it should be better to create separate regular test for it. We dont really need all drivers to be tested for hydratation modes imo. Also, you cannot test e.g. HYDRATE_SIMPLEOBJECT in platform one.

@janedbal janedbal mentioned this pull request Jun 28, 2024
@janedbal
Copy link
Contributor

Here is a draft of how it might look like: #590

@ondrejmirtes
Copy link
Member

Superseded by #590

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.

3 participants