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

feat: eloquent factories (primarily for tests) #3982

Merged
merged 8 commits into from
May 3, 2024

Conversation

SychO9
Copy link
Member

@SychO9 SychO9 commented Apr 27, 2024

Please ignore the failing PHPStan and Frontend tests, those have been fixed in other PRs that are still open

Changes proposed in this pull request:
This PR introduces some eloquent factories for some models. The primary reason for introducing this is the following use case within tests:

We have a very large number of tests, in most tests we declare the data we need in the database like so:

$this->prepareDatabase([
    'discussions' => [
        ['id' => 1, 'title' => 'no tags', 'user_id' => 1, 'comment_count' => 1],
        ['id' => 2, 'title' => 'has tags where mods can view discussions but not flags', 'user_id' => 1, 'comment_count' => 1],
        ['id' => 3, 'title' => 'has tags where mods can view flags but not discussions', 'user_id' => 1, 'comment_count' => 1],
        ['id' => 4, 'title' => 'has tags where mods can view discussions and flags', 'user_id' => 1, 'comment_count' => 1],
        ['id' => 5, 'title' => 'has unrestricted tag', 'user_id' => 1, 'comment_count' => 1],
    ],
]);

We set `comment_count' to 1 almost always rather than not, so that we are certain the discussions are visible. The problem is that when you introduce a new column that requires you to similarly always give it a value, you have to go through all the data in all the tests and make changes accordingly.. Which is very inconvenient.

By introducing factories, each model has a default template of fake data that can be edited, and the data specified in the tests is passed to the model factory to generate the final data.

So with this PR, it becomes as follows:

$this->prepareDatabase([
    Discussion::class => [
        ['id' => 1, 'title' => 'no tags', 'user_id' => 1],
        ['id' => 2, 'title' => 'has tags where mods can view discussions but not flags', 'user_id' => 1],
        ['id' => 3, 'title' => 'has tags where mods can view flags but not discussions', 'user_id' => 1],
        ['id' => 4, 'title' => 'has tags where mods can view discussions and flags', 'user_id' => 1],
        ['id' => 5, 'title' => 'has unrestricted tag', 'user_id' => 1],
    ],
]);

// each row will be passed to the model factory like so internally:
$row = $modelClass::factory()->raw($row);

// The factory will have the default fake value needed.
class DiscussionFactory extends Factory
{
    public function definition(): array
    {
        return [
            'title' => $this->faker->sentence,
            'comment_count' => 1,
            ...
        ];
    }
}

To implement a factory class, the model must use the HasFactory trait, and the factory class which extends Illuminate\Database\Eloquent\Factories\Factory can exist in the same namespace as the model class. Otherwise the model may specify the class of the factory through the static newFactory method.

More on factories here: https://laravel.com/docs/10.x/eloquent-factories.

Additionally, factories are useful for other features the ecosystem may build (an example can involve a previewing feature of sorts).

Reviewers should focus on:

Screenshot

QA

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@SychO9 SychO9 requested a review from a team as a code owner April 27, 2024 15:08
@SychO9 SychO9 added this to the 2.0 milestone Apr 27, 2024
@SychO9 SychO9 self-assigned this Apr 27, 2024
Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Sounds good! Do we need / want to "simplify" the existing sample data in tests?

@SychO9
Copy link
Member Author

SychO9 commented May 3, 2024

Do we need / want to "simplify" the existing sample data in tests?

We don't need to, though it would be nice, will either do that incrementally or in a swoop in one PR. I'll merge this for now though since I need it for the database drivers branch (sqlite + pgsql)

@SychO9 SychO9 merged commit 2b91737 into sqlite-driver May 3, 2024
25 of 29 checks passed
@SychO9 SychO9 deleted the sm/eloquent-factories branch May 3, 2024 08:20
@SychO9 SychO9 restored the sm/eloquent-factories branch May 3, 2024 08:22
@SychO9 SychO9 deleted the sm/eloquent-factories branch May 3, 2024 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants