chelout/laravel-relationship-events

Using HasRelationshipObservables trait in multiple models causes double events to be fired in tests

BoyeMagnus opened this issue · 7 comments

When adding the HasRelationshipObservables trait to multiple models, the tests often fail, because of observers receiving the event twice.

For eg. a user and a group model with HasRelationshipObservables. With two observers that subscribe to belongsToManyAttached both.

trait HasRelationshipObservables
{
    /**
     * @var array
     */
    protected static $relationshipObservables = [];

    /**
     * Initialize relationship observables.
     *
     * @return void
     */
    public static function bootHasRelationshipObservables()
    {
         ......gets the methods

        static::mergeRelationshipObservables($methods);
    }

Static properties will most of the time in tests never be reset, because of how PHPUnit works. When booting HasRelationshipObservables, then it merges the $methods with the static $relationshipObservables. This causes the observer to be fire multiple times in tests where the static property is never reset.

A solution would be to set $relationshipObservables to an empty array in the bootHasRelationshipObservables. Anyone expiring the same issues?

Thanks, @BoyeMagnus
Could you please provide a test that fails?

Yes I'll provide one asap 😄

Thanks again, i'll look into it tomorrow.

Sorry for the delay, had a pretty rough sprint.
I've made a branch with a test job that is being run every time the event should be happening (In the UserObserver). As you can see the job is being fired more than one time if you have test cases in PHPUnit.

Adding static::$relationshipObservables = []; to line 28 in Chelout\RelationshipEvents\Traits\HasRelationshipObservables fixes the problem.
The problem is that the User model is booted multiple times during the tests, and as the package appends the "methods" in the static $relationshipObservables variable, then it keeps growing for each test.

I've created a PR with the test here: #41

@BoyeMagnus sorry for the huge delay, working on MVP project for days and nights 🤯
I would accept PR with described changes with provided tests if you have a little more time then me.