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

tests: automatically create cascade persist permutations #666

Draft
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

nikophil
Copy link
Member

@nikophil nikophil commented Jul 3, 2024

fixes #597

This PR introduces a way to automatically create all permutations for "cascade persist" relationships.

By using a combination of the trait WithEntityRelationShip, and the attribute UsingRelationShips, a data provider is automatically created with all possible combinations of the "cascade persist" and "standard" associations.

ex:

final class EntityRelationshipTest extends KernelTestCase
{
    use WithEntityRelationShip, Factories, ResetDatabase;

    // a matrix with all relationships will be created. Here, for example, there will be 4 permutations:
    // [`Contact::$category` => "cascade", `Category::$contacts` => "cascade"]
    // [`Contact::$category` => "standard", `Category::$contacts` => "cascade"]
    // [`Contact::$category` => "cascade", `Category::$contacts` => "standard"]
    // [`Contact::$category` => "standard", `Category::$contacts` => "standard"]
    #[UsingRelationShips(Contact::class, ['category'])]
    #[UsingRelationShips(Category::class, ['contacts'])]
    /**
     * @test
     * @dataProvider provideCascadeRelationshipsCombination
     */
    public function many_to_one(): void
    {
        // add some test which used `Contact::$category` and `Category::$contacts`
    }
}

If we're OK on this implementation, I think I can remove all StandardXXX and CascadeXXX classes!

Here are some thoughts about this work:

  • We must add the data provider on each method using the attribute UsingRelationShips. I don't think it is possible to automatically add it, but I've added a check which enforces to add the provider if the attribute is used
  • It is currently not possible to have another data provider. But for now, the tests about "orm relationships" do not use data providers, so it's good enough
  • the trait WithEntityRelationShip MUST be before Factories trait. I have to find a way to enforce that. It is a problem of order in the @before hooks and this is why I decided to propose this change in PHPUnit
  • I do think we should test ALL these permutations (I mean, the full matrix, with full cases), we may find some weird edge cases

@nikophil nikophil marked this pull request as draft July 3, 2024 17:38
$temp[$j] = new DoctrineCascadeRelationshipMetadata(
class: $permutations[$j]['class'],
field: $permutations[$j]['field'],
cascade: (bool) (($i >> $j) & 1)
Copy link
Member Author

Choose a reason for hiding this comment

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

thank you chatGPT 😅

Comment on lines +31 to +33
#[UsingRelationShips(StandardContact::class, ['category'])]
#[UsingRelationShips(StandardCategory::class, ['contacts'])]
public function many_to_one(): void
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'm not sure for this specific test if the inverse relationship is even used 🤔

/**
* @return iterable<list<DoctrineCascadeRelationshipMetadata>>
*/
private static function provideCascadeRelationshipsCombination(string $methodName): iterable
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 need to add some tests for this stuff as well

* @author Nicolas PHILIPPE <[email protected]>
*/
#[\Attribute(\Attribute::TARGET_METHOD | \Attribute::IS_REPEATABLE)]
final class UsingRelationShips
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
final class UsingRelationShips
final class UsingRelationships

/**
* @author Nicolas PHILIPPE <[email protected]>
*/
trait WithEntityRelationShip
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
trait WithEntityRelationShip
trait WithEntityRelationship

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

Successfully merging this pull request may close these issues.

[2.x] Automatically test all "cascade" combinations in tests
2 participants