Introduce assertQueryLogTail() for better tests #9890
Conversation
Right now, you get an exception when trying to get a log line that is out of bounds. It is hard to tell what the query log looks like when that happens.
greg0ire
force-pushed
the
assert-query-log-tail
branch
from
c2df07e to
e5aba92
Compare
| 'ALTER TABLE gh6823_user ADD CONSTRAINT FK_70DD1774FE54D947 FOREIGN KEY (group_id) REFERENCES gh6823_group (id)', | ||
| 'ALTER TABLE gh6823_user ADD CONSTRAINT FK_70DD17746BF700BD FOREIGN KEY (status_id) REFERENCES gh6823_status (id)', | ||
| 'ALTER TABLE gh6823_user_tags ADD CONSTRAINT FK_596B1281A76ED395 FOREIGN KEY (user_id) REFERENCES gh6823_user (id)' | ||
| ); |
There was a problem hiding this comment.
This test does not exist on 2.12.x and is the main reason I want to do this. I'm not targeting 2.12.x in order to make the merge up less complicated.
| 'SELECT t0.id AS id_1, t0.driver_id AS driver_id_2, t3.id AS id_4, t3.name AS name_5, t0.owner_id AS owner_id_6, t7.id AS id_8, t7.name AS name_9 FROM Train t0 LEFT JOIN TrainDriver t3 ON t0.driver_id = t3.id INNER JOIN TrainOwner t7 ON t0.owner_id = t7.id WHERE t0.id = ?', | ||
| $this->getLastLoggedQuery()['sql'] | ||
| $this->assertQueryLogTail( | ||
| 'SELECT t0.id AS id_1, t0.driver_id AS driver_id_2, t3.id AS id_4, t3.name AS name_5, t0.owner_id AS owner_id_6, t7.id AS id_8, t7.name AS name_9 FROM Train t0 LEFT JOIN TrainDriver t3 ON t0.driver_id = t3.id INNER JOIN TrainOwner t7 ON t0.owner_id = t7.id WHERE t0.id = ?' |
There was a problem hiding this comment.
Does the ORM own any of these queries? What is the point of making assertions against SQL in functional tests? Given the experience of implementing compatibility with DBAL 4, I'd say that these tests cause more harm than good.
There was a problem hiding this comment.
I see how that test isn't great, and anyone taking a look at ORM 3.x will see how it can get worse. Maybe making it easier to write such tests isn't a great idea. 🤔
What would be the alternative? Using the schema manager to get the charset and collation, and perform assertions on that?
There was a problem hiding this comment.
Are you talking about this specific test (testEagerLoadWithNullableColumnsGeneratesLeftJoinOnBothSides())? I don't see anything related to the charset and collation here.
Whatever the test is, the SQL solves a problem that the API consumer expects to be solved according to the API contract (e.g. the characters in the certain charset are stored/fetched correctly and are sorted according to the collation). A functional test should cover the consumer-facing API contract.
There was a problem hiding this comment.
Sorry, I was referring to the testCharsetCollationWhenCreatingForeignRelations, the test that prompted me to start this PR. I failed to notice your comment was on another test, but I suppose it applies to all of them.
There was a problem hiding this comment.
@morozov I've started taking a look at this. For this one, I'm asserting that you can find a train without driver and vice versa
For the one below though, I'm not sure what to do: you cannot even persist a waggon that does not have a train. Should I test that we get a Doctrine\DBAL\Exception\NotNullConstraintViolationException? Or should I remove the test? After all, if the train id is mandatory, then it shouldn't matter whether we have a LEFT JOIN or an INNER JOIN…
There was a problem hiding this comment.
Maybe this test should use some different model where the referenced object is optional?
There was a problem hiding this comment.
From the documentation:
Whenever you query for an entity that has persistent associations and these associations are mapped as EAGER, they will automatically be loaded together with the entity being queried and is thus immediately available to your application.
I believe it implies that such entities need to be always loaded via a left join since otherwise, they won't load if the associated object is missing. But the test is named testEagerLoadWithNonNullableColumnsGeneratesInnerJoinOnOwningSide(). Why does the ORM need to generate an inner join? This is semantically incorrect.
There was a problem hiding this comment.
Maybe this test should use some different model where the referenced object is optional?
I believe that's what the first test (testEagerLoadWithNullableColumnsGeneratesLeftJoinOnBothSides) is about.
Why does the ORM need to generate an inner join? This is semantically incorrect.
Since the referencing column is non nullable, there is a guarantee that the object exists so it shouldn't matter whether this is a LEFT JOIN or an INNER JOIN IMO.
There was a problem hiding this comment.
It still doesn’t answer the question why, and it’s not entirely correct. Without an FK, there’s no guarantee that there always will be a match in the referenced table.
|
Closing in favor of #9895 |
Right now, you get an exception when trying to get a log line that is
out of bounds. It is hard to tell what the query log looks like when
that happens.
I decided to work on this after facing that issue on #9885