Skip to content

Conversation

@olivernybroe
Copy link
Contributor

This PR adds a phpunit comparator for models.

The comparator compares the models based on the model is method.
Phpunit comparators are used for the methods assertEquals and assertNotEquals.

I often encounter that I want to check if two models are the same, and then use the assertEquals, only to get an error about things not being identical. This is often caused by fields like the wasRecentlyCreated and so on.
To get around this I normally write assertTrue($model->is($otherModel)).

This solution might be BC and should have been submitted to L9, as it changes the current assertEquals result, however submitting it to L8 because it uses the official is method which is used for comparing models. If you consider this a BC i'll submit to L9 instead 👍

Another solution for this would be so instead of using the is method for comparison it uses the getAttributes arrays for comparison.

@GrahamCampbell GrahamCampbell changed the title Add a phpunit comparator for models [8.x] Add a phpunit comparator for models Aug 16, 2021
@driesvints
Copy link
Member

Hi, I merged #38398 into 8.x to get the test suite passing again. I've put your PR in draft. Please rebase your PR against 8.x to make the tests of this PR pass. After you've done that and tests pass, mark this PR as ready for review. Thanks.

@driesvints driesvints marked this pull request as draft August 16, 2021 10:27
@olivernybroe olivernybroe force-pushed the feat-model-comparator branch from c785492 to ea4aacb Compare August 16, 2021 10:40
@olivernybroe olivernybroe marked this pull request as ready for review August 16, 2021 10:58
@taylorotwell
Copy link
Member

Yeah - not sure I want to do this on a patch release. Can't even decide if I want to do it at all atm 😅

@olivernybroe
Copy link
Contributor Author

Yeah, prob. shouldn't be patch 😄
Thought I would open a PR for hearing the teams thoughts about it 👍

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.

3 participants