Move tests requiring PHP ≥ 8.1 to their own directory

[MatTheCat]

Follow #10666 (comment) (and #10662 (comment)).

I mimicked doctrine/persistence by creating a tests_php81 alongside tests.

[greg0ire]

I'm on my phone but this looks exactly like what's needed. We'll have to decide what to do on 3.0.x. Merging up could be painful if we move files back to the main directory, and then modify them on 2.x. I don't know exactly how git behaves in that kind of case.

[MatTheCat]

Nice 👌

I’m just not sure which folders/namespaces you want as it is different between doctrine/persistence and doctrine/reflection.

[greg0ire]

The namespace of this file (can't comment directly on it) is wrong now, right?

[greg0ire]

I’m just not sure which folders/namespaces you want as it is different between doctrine/persistence and doctrine/reflection.

I'm fine with what you did 👍

[greg0ire]

Codecov says there is a drop in code coverage… the code no longer considered covered has to do with enums. Either the tests are not running, or they are somehow not included in the report.

EDIT: it seems to be the latter.

Also, I have the following error when running the tests:

There was 1 error:

1) Doctrine\Tests_PHP81\ORM\Functional\Ticket\GH10132Test::testQueryBackedEnumInCompositeKeyJoin
Doctrine\Common\Annotations\AnnotationException: [Semantical Error] The annotation "@Entity" in class Doctrine\Tests_PHP81\ORM\Functional\Ticket\Complex was never imported. Did you maybe forget to add a "use" statement for this annotation?

/home/greg/dev/doctrine-orm/patch/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/AnnotationException.php:36
/home/greg/dev/doctrine-orm/patch/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php:790
/home/greg/dev/doctrine-orm/patch/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php:712
/home/greg/dev/doctrine-orm/patch/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php:368
/home/greg/dev/doctrine-orm/patch/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/AnnotationReader.php:142
/home/greg/dev/doctrine-orm/patch/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/PsrCachedReader.php:155
/home/greg/dev/doctrine-orm/patch/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/PsrCachedReader.php:57
/home/greg/dev/doctrine-orm/patch/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php:90
/home/greg/dev/doctrine-orm/patch/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php:136
/home/greg/dev/doctrine-orm/patch/vendor/doctrine/persistence/src/Persistence/Mapping/AbstractClassMetadataFactory.php:343
/home/greg/dev/doctrine-orm/patch/vendor/doctrine/persistence/src/Persistence/Mapping/AbstractClassMetadataFactory.php:207
/home/greg/dev/doctrine-orm/patch/lib/Doctrine/ORM/EntityManager.php:318
/home/greg/dev/doctrine-orm/patch/tests/Doctrine/Tests/OrmFunctionalTestCase.php:377
/home/greg/dev/doctrine-orm/patch/tests/Doctrine/Tests/OrmFunctionalTestCase.php:375
/home/greg/dev/doctrine-orm/patch/tests/Doctrine/Tests/OrmFunctionalTestCase.php:347
/home/greg/dev/doctrine-orm/patch/tests_php81/ORM/Functional/Ticket/GH10132Test.php:18

[greg0ire]

There is indeed no import statement for this… why does the test suite pass when run on Github 🤔 ?

EDIT: Github says it ran 3689 tests… locally, I have 3722 tests.

[greg0ire]

💡 I think you need to modify all of these files: https://github.com/doctrine/orm/tree/2.15.x/ci/github/phpunit

[MatTheCat]

Thanks for the review @greg0ire I'll look into this!

[derrabus]

I honestly don't think that this change is a good idea. We're moving tests to a separate directory just to move them back in 3.0.

[MatTheCat]

I tend to agree, especially if moving stubs into their own classes is enough to make 2.x CI green.

Waiting for a consensus then.

[greg0ire]

What I don't like with that solution is that the result is not cohesive. Files are scattered in 2 different directories:

• tests/Doctrine/Tests/ORM/Functional/Ticket
• tests/Doctrine/Models

In my opinion:

• We should not reuse models between tests, because changing the model for one test might break another test.
• We should make it difficult to remove tests without removing the corresponding models.

This could be achieved by having both the test and its model live in the same namespace/directory, named after the ticket. To take the example of #10666, we would have:

GH10132
├── ClassMetadataInfoTest.php
├── ComplexChild.php
└── Complex.php

As a consequence, the test would be more legible as there would not be a need for prefixes, and we would need to find a name for the test file. Here I picked the named of the class under test, which I think is nice. Can give us some insight on what tests cover which class.

If we go that way, we should document here that the other way (the non cohesive one) is deprecated and that new tests should follow the new way.

Regarding this move being a bad idea, I think I know what you mean:

• Either we don't move the tests in 3.0.x to avoid conflicts (until we drop support for 2.x, that is), and then we have a separation that we don't need;
• Or we get difficult merges up.

[derrabus]

We should not reuse models between tests, because changing the model for one test might break another test.

We have multiple sets of models where reusing them makes sense, like the CMS and the Company sets. Tests that use them are easier to comprehend because I as a human reader don't need to learn a new model for each test case.

But of course we should refrain from changing those models too often. And that makes them a bad fit for reproducing edge cases. Which is why I believe we need a good mix of both, generic reusable models and specific models that are meant to be used by a single functional test only.

We should make it difficult to remove tests without removing the corresponding models.

And we should make it difficult to reuse models that are tied to a different test. Maybe a custom PHPStan rule could help here.

This could be achieved by having both the test and its model live in the same namespace/directory, named after the ticket. To take the example of #10666, we would have:

GH10132
├── ClassMetadataInfoTest.php
├── ComplexChild.php
└── Complex.php

Sounds good.

Regarding this move being a bad idea, I think I know what you mean:

* Either we don't move the tests in 3.0.x to avoid conflicts (until we drop support for 2.x, that is), and then we have a separation that we don't need;

* Or we get difficult merges up.

Exactly.

[greg0ire]

Let's close this then.

If we go that way, we should document here that the other way (the non cohesive one) is deprecated and that new tests should follow the new way.

Let's not deprecate anything, and just document both ways: either the contributor should reuse an existing model without modifying it, or they should design their own tests, in which case they should use the cohesive solution.

[greg0ire]

Thanks @MatTheCat , and sorry for all this back and forth!

[MatTheCat]

No problem, will update #10666 accordingly 👌