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 | Show request endpoint in MockClient exception message #269

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

Joel-Jensen
Copy link
Contributor

PR for my feature request in #219 (comment)

Displays the missing endpoint in the exception for easier debugging tests.

How Laravel, Http::preventStrayRequest() does it:
image

This PR:
image

Before this PR:
image

@what-the-diff
Copy link

what-the-diff bot commented Aug 9, 2023

PR Summary 😊

  • Added More Info to Errors 🧐
    When our system can't imitate a certain request (in other words "mock" it), the kind of error we see wasn't too helpful before. Now we have tinkered around and added a way for these errors to show more about the confusing request that caused them. So next time an un-mockable request appears, we will have more clues to understand why! 🕵️

  • Improved Error Triggering in 'GuessNextResponse' 👍
    In our code, there's a method known as guessNextResponse (Kind of like guessing the next move in a chess game, right? 😄). Previously, when it encountered problems, it didn't hand over the complete details about the concerning request. But from now on, it's going to pass along the full info about the request that caused the hiccup to the error handler, assisting us to debug more effectively. 🐞 🔍

@Sammyjo20 Sammyjo20 changed the title Display missing endpoint Feature | Show request endpoint in MockClient exception message Aug 10, 2023
Copy link
Member

@Sammyjo20 Sammyjo20 left a comment

Choose a reason for hiding this comment

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

Looks great, thank you @Joel-Jensen. Would you mind checking over PHP-Stan as well as update any tests to include this? Thank you!

@juse-less
Copy link
Contributor

@Joel-Jensen The only thing you should need to do is rebase from latest v3, where Sam recently downgraded the PHP constraint (8.2 -> 8.1) in composer.json.
Then PHPStan should stop complaining.

I can't see anything that PHPStan should complain about in your actual code changes. 👍

@Joel-Jensen
Copy link
Contributor Author

Seems like that branch isn't that stable right now, should I just merge this to v2?

@Sammyjo20
Copy link
Member

@Joel-Jensen I can get the tests passing for v3 - I'll sort this a little bit later. Happy to leave PHPStan for now as v3 isn't ready yet

@Sammyjo20
Copy link
Member

If you catch up with master now, it should pass 👍

@Sammyjo20 Sammyjo20 merged commit 858fbc7 into saloonphp:v3 Sep 5, 2023
9 of 10 checks passed
@Sammyjo20
Copy link
Member

Thanks @Joel-Jensen !

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