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

Feature | Allow merging data in mocked fixture responses #425

Open
wants to merge 4 commits into
base: v3
Choose a base branch
from

Conversation

innocenzi
Copy link

Closes #416

This pull request allows merging data in fixtures:

MockResponse::fixture('user', merge: [
    'name' => 'Sam Carré',
])

This is useful when the mocked response is used by other part of the code that depends on it, and assertions are performed later on the result of that code.

One (very) simple example of that would be testing if a data transfer object can be created:

$response = connector()->send(new DTORequest, new MockClient([
    MockResponse::fixture('user', merge: [
        'name' => 'Sam Carré',
    ]),
]));

expect($response->dto())
    ->name->toBe('Sam Carré') // Sam Carré instead of Sam
    ->actualName->toBe('Sam')
    ->twitter->toBe('@carre_sam');

@innocenzi
Copy link
Author

One small concern I have is that this code uses array_merge, while I'm certain to have use cases where array_merge_recursive would be more useful.

I wanted to stay consistent with implementations of MergeableBody which all use array_merge. Would you be open to a PR that updates all of these to array_merge_recursive?

@innocenzi
Copy link
Author

@Sammyjo20 do you think you could take a look at that? this would really ease testing requests 🙏

@innocenzi
Copy link
Author

You can amend my previous concern, I refactored the pull request to support dot-notation, which fixes the issue.

I've been testing this in a work project, array_merge was not enough most of the time, and I needed dot-notation support for most fixtures.

@Sammyjo20
Copy link
Member

Hey @innocenzi thank you for this PR, it looks like a great addition! The only thing I ask is instead of having it as a second argument could we have a method that chains onto the ::fixture() call? Like MockResponse::fixture()->merge([]) because I've got an upcoming feature that will also share this API so it would be good if it was unified. What do you think?

@innocenzi
Copy link
Author

innocenzi commented Jun 26, 2024

That's a nice idea—I updated the PR!

@innocenzi
Copy link
Author

@Sammyjo20 any chance you could take another look at this?

@Sammyjo20 Sammyjo20 changed the title feat(testing): allow merging data in mocked fixture responses Feature | Allow merging data in mocked fixture responses Dec 3, 2024
@Sammyjo20
Copy link
Member

Hi @innocenzi sorry about the delay. I'm going through all of Saloon's issues and PRs again at the moment and I will update accordingly.

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.

Allow partial overriding of fixtures
3 participants