Skip to content

[12.x] Allow globally disabling Factory parent relationships via Factory::dontExpandRelationshipsByDefault() #56154

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

Open
wants to merge 4 commits into
base: 12.x
Choose a base branch
from

Conversation

cosmastech
Copy link
Contributor

@cosmastech cosmastech commented Jun 27, 2025

A continuation of #53450. This feature was added by @browner12 and it's been incredibly helpful for our team in speeding up tests.

That said, it requires us to write ->withoutParents() on each factory make, which has led to bugs. Namely, we don't add the trait to refresh the database and then data gets left over between tests, which breaks expectations in other tests... but we don't discover it for a month 😆 😭 and then it's debug city.

It would be great if in the test's setup, we can just indicate no factory should create a parent relationship.

public function test_has_one_editor_permission_returns_true(): void
{
+    UserPermissionFactory::dontExpandRelationshipsByDefault();
+
    $collection = new UserPermissionCollection([
        UserPermission::factory()
-            ->withoutParents()
            ->make([
                'type' => 'viewer',
                'company_id' => 2,
                'product_id' => 789,
            ]),
        UserPermission::factory()
-            ->withoutParents()
            ->make([
                'type' => 'editor',
                'company_id' => 2,
                'product_id' => 432,
            ]),
    ]);

    $result = $collection->hasEditorForCompany(2);

    $this->assertTrue($result);
}

Even better, I would just add this to the class's setUp() method.

@NickSdot
Copy link
Contributor

Would a descriptive method be more Laravel-y?

UserPermissionFactory::disableExpandedRelationships()

@DarkGhostHunter
Copy link
Contributor

DarkGhostHunter commented Jul 4, 2025

This gave me an idea. What if we could declare the methods to call when invoking a given Factory inside the test? Something like this:

#[UserFactory('withoutParents', 'someOtherThing')]
class TestSomething extends TestCase
{
    // ...
}

Then, instead of replicating all calls in each test, only call the method on top of the class and call it a day.

My guess is that the Test Case would put these methods on a static array that would be called each time the Factory is instantiated, and flushed once the test case ends.

@cosmastech
Copy link
Contributor Author

This gave me an idea. What if we could declare the methods to call when invoking a given Factory inside the test? Something like this:

#[UserFactory('withoutParents', 'someOtherThing')]
class TestSomething extends TestCase
{
    // ...
}

Then, instead of replicating all calls in each test, only call the method on top of the class and call it a day.

My guess is that the Test Case would put these methods on a static array that would be called each time the Factory is instantiated, and flushed once the test case ends.

That's an interesting idea, though to me it feels like a pretty limited use-case 🤷 What other factory methods do you think a user might set via this attribute? And would it require userland to create an attribute for each factory? Maybe #[FactorySettings(UserFactory::class, ['withoutParents'])] would be less code?

I would prefer to see a way to setup the database once for all tests in a test case. That to me seems like a much more common problem, which would improve test run time.

Take for instance all of my test cases inside of a single class require a user, an organization, a user permission for that organization, and a team. These have to be built for each test method, and then are torn down at the end. Would love if there was a way to build that data once for all tests, run each test so they can leverage those common values, and then remove it once we're done with all tests in the class.

This is a totally separate issue to the one I'm proposing here, but just thinking that this would get some serious mileage in the codebase I am working on. I don't fully understand how parallel testing runs via paratest, so maybe this is a non-starter based on that. 🤔

@cosmastech
Copy link
Contributor Author

Would a descriptive method be more Laravel-y?

UserPermissionFactory::disableExpandedRelationships()

You're right. I had that thought, but came across these issues:

  • What is a good name for it? It's called withoutParents as an instance method, so the static method should be something similar (defaultWithParents() maybe?) Naming it disableExpandedRelationships(), you need a way to re-enable it as well. So it would take a bool as an argument. Feels weird to make the negative case the only way to access it. 🤷
  • It would be used in tests mostly, so I figured that giving it just a public static property was probably decent enough DX.

I'll wait to see what Taylor's thoughts are on this. Happy to make whatever changes are necessary.

@taylorotwell
Copy link
Member

I would do something like dontExpandRelationshipsByDefault.

@taylorotwell taylorotwell marked this pull request as draft July 7, 2025 14:33
@cosmastech
Copy link
Contributor Author

cosmastech commented Jul 7, 2025

I would do something like dontExpandRelationshipsByDefault.

@taylorotwell do you mean a static function with that name?

@cosmastech cosmastech force-pushed the static-without-parents branch from bf839be to 4291fc6 Compare July 7, 2025 16:04
@cosmastech cosmastech changed the title [12.x] Allow globally disabling Factory parent relationships [12.x] Allow globally disabling Factory parent relationships via Factory:: dontExpandRelationshipsByDefault() Jul 7, 2025
@cosmastech cosmastech marked this pull request as ready for review July 7, 2025 16:05
@cosmastech cosmastech changed the title [12.x] Allow globally disabling Factory parent relationships via Factory:: dontExpandRelationshipsByDefault() [12.x] Allow globally disabling Factory parent relationships via Factory::dontExpandRelationshipsByDefault() Jul 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants