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.

Sign up for GitHub

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

Introduce assertQueryLogTail() for better tests #9890

Closed
wants to merge 1 commit into from
Closed

Introduce assertQueryLogTail() for better tests #9890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 10 additions & 18 deletions tests/Doctrine/Tests/ORM/Functional/OneToOneEagerLoadingTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,16 +153,14 @@ public function testEagerLoadWithNullableColumnsGeneratesLeftJoinOnBothSides():
$this->_em->clear();

$train = $this->_em->find(get_class($train), $train->id);
$this->assertSQLEquals(
'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 = ?'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@morozov morozov Jul 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@greg0ire greg0ire Jul 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this test should use some different model where the referenced object is optional?

Copy link
Member

@morozov morozov Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);

$this->_em->clear();
$driver = $this->_em->find(get_class($driver), $driver->id);
$this->assertSQLEquals(
'SELECT t0.id AS id_1, t0.name AS name_2, t3.id AS id_4, t3.driver_id AS driver_id_5, t3.owner_id AS owner_id_6 FROM TrainOwner t0 LEFT JOIN Train t3 ON t3.owner_id = t0.id WHERE t0.id IN (?)',
$this->getLastLoggedQuery()['sql']
$this->assertQueryLogTail(
'SELECT t0.id AS id_1, t0.name AS name_2, t3.id AS id_4, t3.driver_id AS driver_id_5, t3.owner_id AS owner_id_6 FROM TrainOwner t0 LEFT JOIN Train t3 ON t3.owner_id = t0.id WHERE t0.id IN (?)'
);
}

Expand All @@ -183,16 +181,11 @@ public function testEagerLoadWithNonNullableColumnsGeneratesInnerJoinOnOwningSid

$waggon = $this->_em->find(get_class($waggon), $waggon->id);

// The last query is the eager loading of the owner of the train
$this->assertSQLEquals(
'SELECT t0.id AS id_1, t0.name AS name_2, t3.id AS id_4, t3.driver_id AS driver_id_5, t3.owner_id AS owner_id_6 FROM TrainOwner t0 LEFT JOIN Train t3 ON t3.owner_id = t0.id WHERE t0.id IN (?)',
$this->getLastLoggedQuery()['sql']
);

// The one before is the fetching of the waggon and train
$this->assertSQLEquals(
$this->assertQueryLogTail(
// The one before is the fetching of the waggon and train
'SELECT t0.id AS id_1, t0.train_id AS train_id_2, t3.id AS id_4, t3.driver_id AS driver_id_5, t3.owner_id AS owner_id_6 FROM Waggon t0 INNER JOIN Train t3 ON t0.train_id = t3.id WHERE t0.id = ?',
$this->getLastLoggedQuery(1)['sql']
// The last query is the eager loading of the owner of the train
'SELECT t0.id AS id_1, t0.name AS name_2, t3.id AS id_4, t3.driver_id AS driver_id_5, t3.owner_id AS owner_id_6 FROM TrainOwner t0 LEFT JOIN Train t3 ON t3.owner_id = t0.id WHERE t0.id IN (?)'
);
}

Expand All @@ -208,9 +201,8 @@ public function testEagerLoadWithNonNullableColumnsGeneratesLeftJoinOnNonOwningS
$this->_em->clear();

$waggon = $this->_em->find(get_class($owner), $owner->id);
$this->assertSQLEquals(
'SELECT t0.id AS id_1, t0.name AS name_2, t3.id AS id_4, t3.driver_id AS driver_id_5, t3.owner_id AS owner_id_6 FROM TrainOwner t0 LEFT JOIN Train t3 ON t3.owner_id = t0.id WHERE t0.id = ?',
$this->getLastLoggedQuery()['sql']
$this->assertQueryLogTail(
'SELECT t0.id AS id_1, t0.name AS name_2, t3.id AS id_4, t3.driver_id AS driver_id_5, t3.owner_id AS owner_id_6 FROM TrainOwner t0 LEFT JOIN Train t3 ON t3.owner_id = t0.id WHERE t0.id = ?'
);
}

Expand Down
9 changes: 2 additions & 7 deletions tests/Doctrine/Tests/ORM/Functional/OneToOneInverseSideLoadAfterDqlQueryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,9 @@ public function testInverseSideOneToOneLoadedAfterDqlQuery(): void
self::assertInstanceOf(InverseSide::class, $fetchedInverse);
self::assertInstanceOf(OwningSide::class, $fetchedInverse->owning);

$this->assertSQLEquals(
$this->assertQueryLogTail(
'select o0_.id as id_0 from one_to_one_inverse_side_load_inverse o0_ where o0_.id = ?',
$this->getLastLoggedQuery(1)['sql']
);

$this->assertSQLEquals(
'select t0.id as id_1, t0.inverse as inverse_2 from one_to_one_inverse_side_load_owning t0 WHERE t0.inverse = ?',
$this->getLastLoggedQuery()['sql']
'select t0.id as id_1, t0.inverse as inverse_2 from one_to_one_inverse_side_load_owning t0 WHERE t0.inverse = ?'
);
}
}
15 changes: 6 additions & 9 deletions tests/Doctrine/Tests/ORM/Functional/Ticket/DDC1595Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,26 +49,23 @@ public function testIssue(): void
$entity1 = $repository->find($e1->id);

// DDC-1596
$this->assertSQLEquals(
"SELECT t0.id AS id_1, t0.type FROM base t0 WHERE t0.id = ? AND t0.type IN ('Entity1')",
$this->getLastLoggedQuery()['sql']
$this->assertQueryLogTail(
"SELECT t0.id AS id_1, t0.type FROM base t0 WHERE t0.id = ? AND t0.type IN ('Entity1')"
);

$entities = $entity1->getEntities()->getValues();

self::assertEquals(
"SELECT t0.id AS id_1, t0.type FROM base t0 INNER JOIN entity1_entity2 ON t0.id = entity1_entity2.item WHERE entity1_entity2.parent = ? AND t0.type IN ('Entity2')",
$this->getLastLoggedQuery()['sql']
self::assertQueryLogTail(
"SELECT t0.id AS id_1, t0.type FROM base t0 INNER JOIN entity1_entity2 ON t0.id = entity1_entity2.item WHERE entity1_entity2.parent = ? AND t0.type IN ('Entity2')"
);

$this->_em->clear();

$entity1 = $repository->find($e1->id);
$entities = $entity1->getEntities()->count();

$this->assertSQLEquals(
'SELECT COUNT(*) FROM entity1_entity2 t WHERE t.parent = ?',
$this->getLastLoggedQuery()['sql']
$this->assertQueryLogTail(
'SELECT COUNT(*) FROM entity1_entity2 t WHERE t.parent = ?'
);
}
}
Expand Down
16 changes: 9 additions & 7 deletions tests/Doctrine/Tests/ORM/Functional/Ticket/GH6823Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,15 @@ public function testCharsetCollationWhenCreatingForeignRelations(): void
GH6823Status::class
);

self::assertEquals('CREATE TABLE gh6823_user (id VARCHAR(255) NOT NULL, group_id VARCHAR(255) CHARACTER SET ascii DEFAULT NULL COLLATE `ascii_general_ci`, status_id VARCHAR(255) CHARACTER SET latin1 DEFAULT NULL COLLATE `latin1_bin`, INDEX IDX_70DD1774FE54D947 (group_id), INDEX IDX_70DD17746BF700BD (status_id), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8mb4 COLLATE `utf8mb4_bin` ENGINE = InnoDB', $this->getLastLoggedQuery(6)['sql']);
self::assertEquals('CREATE TABLE gh6823_user_tags (user_id VARCHAR(255) CHARACTER SET utf8mb4 NOT NULL COLLATE `utf8mb4_bin`, tag_id VARCHAR(255) CHARACTER SET latin1 NOT NULL COLLATE `latin1_bin`, INDEX IDX_596B1281A76ED395 (user_id), INDEX IDX_596B1281BAD26311 (tag_id), PRIMARY KEY(user_id, tag_id)) DEFAULT CHARACTER SET ascii COLLATE `ascii_general_ci` ENGINE = InnoDB', $this->getLastLoggedQuery(5)['sql']);
self::assertEquals('CREATE TABLE gh6823_group (id VARCHAR(255) NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET ascii COLLATE `ascii_general_ci` ENGINE = InnoDB', $this->getLastLoggedQuery(4)['sql']);
self::assertEquals('CREATE TABLE gh6823_status (id VARCHAR(255) CHARACTER SET latin1 NOT NULL COLLATE `latin1_bin`, PRIMARY KEY(id)) DEFAULT CHARACTER SET koi8r COLLATE `koi8r_bin` ENGINE = InnoDB', $this->getLastLoggedQuery(3)['sql']);
self::assertEquals('ALTER TABLE gh6823_user ADD CONSTRAINT FK_70DD1774FE54D947 FOREIGN KEY (group_id) REFERENCES gh6823_group (id)', $this->getLastLoggedQuery(2)['sql']);
self::assertEquals('ALTER TABLE gh6823_user ADD CONSTRAINT FK_70DD17746BF700BD FOREIGN KEY (status_id) REFERENCES gh6823_status (id)', $this->getLastLoggedQuery(1)['sql']);
self::assertEquals('ALTER TABLE gh6823_user_tags ADD CONSTRAINT FK_596B1281A76ED395 FOREIGN KEY (user_id) REFERENCES gh6823_user (id)', $this->getLastLoggedQuery(0)['sql']);
self::assertQueryLogTail(
'CREATE TABLE gh6823_user (id VARCHAR(255) NOT NULL, group_id VARCHAR(255) CHARACTER SET ascii DEFAULT NULL COLLATE `ascii_general_ci`, status_id VARCHAR(255) CHARACTER SET latin1 DEFAULT NULL COLLATE `latin1_bin`, INDEX IDX_70DD1774FE54D947 (group_id), INDEX IDX_70DD17746BF700BD (status_id), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8mb4 COLLATE `utf8mb4_bin` ENGINE = InnoDB',
'CREATE TABLE gh6823_user_tags (user_id VARCHAR(255) CHARACTER SET utf8mb4 NOT NULL COLLATE `utf8mb4_bin`, tag_id VARCHAR(255) CHARACTER SET latin1 NOT NULL COLLATE `latin1_bin`, INDEX IDX_596B1281A76ED395 (user_id), INDEX IDX_596B1281BAD26311 (tag_id), PRIMARY KEY(user_id, tag_id)) DEFAULT CHARACTER SET ascii COLLATE `ascii_general_ci` ENGINE = InnoDB',
'CREATE TABLE gh6823_group (id VARCHAR(255) NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET ascii COLLATE `ascii_general_ci` ENGINE = InnoDB',
'CREATE TABLE gh6823_status (id VARCHAR(255) CHARACTER SET latin1 NOT NULL COLLATE `latin1_bin`, PRIMARY KEY(id)) DEFAULT CHARACTER SET koi8r COLLATE `koi8r_bin` ENGINE = InnoDB',
'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)'
);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

Expand Down
16 changes: 16 additions & 0 deletions tests/Doctrine/Tests/OrmFunctionalTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,13 @@
use Symfony\Component\Cache\Adapter\ArrayAdapter;
use Throwable;

use function array_column;
use function array_map;
use function array_pop;
use function array_reverse;
use function array_slice;
use function assert;
use function count;
use function explode;
use function get_debug_type;
use function getenv;
Expand Down Expand Up @@ -949,6 +951,20 @@ final protected function getLastLoggedQuery(int $index = 0): array
return $lastQuery;
}

/**
* Asserts that the query log finishes with the given SQL queries, modulo case.
*/
final protected function assertQueryLogTail(string ...$expectedQueries): void
{
self::assertSame(
array_map('mb_strtolower', $expectedQueries),
array_map('mb_strtolower', array_column(array_slice(
$this->getQueryLog()->queries,
-1 * count($expectedQueries)
), 'sql'))
);
}

/**
* Drops the table with the specified name, if it exists.
*
Expand Down