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.

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

Review and merge Mapping Drivers tests #2348

Open
IonBazan opened this issue Jul 26, 2021 · 3 comments
Open

Review and merge Mapping Drivers tests #2348

IonBazan opened this issue Jul 26, 2021 · 3 comments
Labels

Comments

@IonBazan
Copy link
Member

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
Copy link
Member

malarzm commented Aug 16, 2021

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 :)

@malarzm malarzm added the Task label Aug 16, 2021
@IonBazan
Copy link
Member Author

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
Copy link
Member

malarzm commented Aug 19, 2021

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants