doctrine / mongodb-odm

The Official PHP MongoDB ORM/ODM
https://www.doctrine-project.org/projects/doctrine-mongodb-odm/en/latest/
MIT License
1.09k stars 504 forks source link

Review and merge Mapping Drivers tests #2348

Open IonBazan opened 3 years ago

IonBazan commented 3 years ago

Improvement Request

Q A
New Feature yes
RFC no
BC Break no

Summary

Currently there are 2 separate namespaces containing Mapping Drivers tests: Doctrine\ODM\MongoDB\Tests\Mapping and Doctrine\ODM\MongoDB\Tests\Mapping\Driver:

The first one derive from https://github.com/doctrine/mongodb-odm/blob/2.2.x/tests/Doctrine/ODM/MongoDB/Tests/Mapping/AbstractMappingDriverTest.php class, tests both AnnotationDriver and XmlDriver, seems newer, but all tested Document classes are placed in the abstract test class file.

The second one is just one test class that extends https://github.com/doctrine/mongodb-odm/blob/2.2.x/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/AbstractDriverTest.php, does just some simple checks, tests only the XML driver, is very old (remembers 2010), but the fixture files are placed in a separate directory: https://github.com/doctrine/mongodb-odm/tree/2.2.x/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/fixtures which makes it much cleaner and easier to refactor.

I believe these tests should be merged together, probably kept only the new classes but fixtures should be extracted into separate files for easier maintainability.

malarzm commented 3 years ago

The first one derive from https://github.com/doctrine/mongodb-odm/blob/2.2.x/tests/Doctrine/ODM/MongoDB/Tests/Mapping/AbstractMappingDriverTest.php class, tests both AnnotationDriver and XmlDriver, seems newer, but all tested Document classes are placed in the abstract test class file.

Placing classes in same file is great solution to be sure they're not reused across tests. Personally I like this approach but am open to changing it given sensible solution :)

The second one is just one test class that extends https://github.com/doctrine/mongodb-odm/blob/2.2.x/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/AbstractDriverTest.php, does just some simple checks, tests only the XML driver, is very old (remembers 2010), but the fixture files are placed in a separate directory: https://github.com/doctrine/mongodb-odm/tree/2.2.x/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/fixtures which makes it much cleaner and easier to refactor.

This test probably remembers time when there also was a YAML metadata driver and its task was to ensure that every mapping option works for both drivers.

I believe these tests should be merged together, probably kept only the new classes but fixtures should be extracted into separate files for easier maintainability.

I will trust your best judgement here as you recently dived into mapping stuff deep enough to add attribute driver :)

IonBazan commented 3 years ago

Placing classes in same file is great solution to be sure they're not reused across tests. Personally I like this approach but am open to changing it given sensible solution :)

You are right but in this case, all the documents should be shared between all driver tests so we can use same assertions on same documents, but with different drivers. Maybe some of them could be placed in the test file itself, but I'd reserve it only for some edge cases or testing driver-specific features (like different ways to map Indexes in AnnotationDriver).

This test probably remembers time when there also was a YAML metadata driver and its task was to ensure that every mapping option works for both drivers.

Yes, it seems that there used to be the YamlDriver test there that compared the results with XmlDriver but since it's gone now, it doesn't really add much value.

I will try to come up with some solution for this. Should I target 2.3 (and higher)?

malarzm commented 3 years ago

I will try to come up with some solution for this. Should I target 2.3 (and higher)?

Please do, let's have attribute driver benefit from this already :)