tarantool-php / client

PHP client for Tarantool.
MIT License
67 stars 22 forks source link

test: fix case sensitive cases in SQL tests #88

Closed AlgebraicWolf closed 1 year ago

AlgebraicWolf commented 1 year ago

The feature will be introduced in Tarantool 3.0 [1].

  1. tarantool/tarantool#9249
rybakit commented 1 year ago

Thanks for the PR! I looked thought the referenced PR and it's not clear to me why these changes are needed. According to this comment, everything should just work as it is now? Is there any documentation explaining the upcoming SQL change in detail?

FYI, there are a few more integration tests using SQL, check out the examples folder. These examples are part of the integration test suite. What confuses me is that if converting everything to upper case is necessary, and your PR is supposed to fix some broken integration pipeline, why aren't these SQL examples causing pipeline failures?

Last but not least, there are also some SQL-related examples in the README. Again, if upper case is mandatory, they need to be updated as well.

ImeevMA commented 1 year ago

I can't speak to examples, but here you can see why this change is necessary. The same comment you linked to said that the CREATE TABLE exec_query (id INTEGER PRIMARY KEY, name VARCHAR(50)); is now creates the exec_query table with id and name columns. Previously, this was an EXEC_QUERY table with ID and NAME columns.

AlgebraicWolf commented 1 year ago

Previously, Tarantool performed normalisation of provided SQL names, translating them to uppercase. This would not be done anymore, instead the names would be used as-is. So, any code that uses a consistent style for names should work fine, for the most part.

The tests in the files, though, feature an exception from this, as they perform checks on metadata (more specifically, it affects testSqlQueryResultHoldsMetadata and testSqlQueryResultHoldsExtendedMetadata). Currently, Tarantool returns names in metadata in a normalised form, but this will no longer hold after an update. This would cause tests to fail, as they expect to get uppercase names, and will instead get them in whatever format they were provided (which is lowercase here).

Fixing the expected value to match the case would not be a portable fix, as it would make the tests fail on the current versions of Tarantool. Therefore, I've updated the tests in the affected test files to use uppercase names to have the same expected values on various versions of Tarantool.

rybakit commented 1 year ago

@ImeevMA @AlgebraicWolf Thanks for the clarification, now I understand the problem.

it affects testSqlQueryResultHoldsMetadata and testSqlQueryResultHoldsExtendedMetadata).

@AlgebraicWolf Is it possible to update only these two failing tests? We can use a dedicated space for testSqlQueryResultHoldsMetadata (similar to testSqlQueryResultHoldsExtendedMetadata) to avoid changing the rest of the tests.

AlgebraicWolf commented 1 year ago

Sure! I'll update the pull request accordingly

AlgebraicWolf commented 1 year ago

@rybakit I've made changes significantly less invasive, only patching the failing tests

rybakit commented 1 year ago

Thank you @AlgebraicWolf!