-
Notifications
You must be signed in to change notification settings - Fork 202
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
Improve reporting when cassette does not contain response #157
base: master
Are you sure you want to change the base?
Conversation
6d35272
to
5503112
Compare
The TravisCI failure seems to be caused by a third party WSDL not being loaded. |
Hi @kasperg , thank you for your contribution. I'll check the WSDL issue and then I'll review your code and rerun the failed tests. I won't find time today, but in a couple of days it will be done. Regards. |
5503112
to
ee6c2a2
Compare
ee6c2a2
to
7913698
Compare
It may be tricky to determine why a recorded response is not used as no information about how the actual request compares to the recorded once. This change changes the behaviour of the playback method to throw an exception is a match is not found. The message contains information about the involved requests. If PHP-VCR is also configured not to allow recording of new requests then the new exception is chained to the existing one. Additional information related to request matching is added to the new root exception.
7913698
to
3dbbaf5
Compare
Politely bumping this. I revisited PHP-VCR and had to debug mismatches between requests and cassettes again. The changes in this PR came in handy again. The WSDL issue is gone and I have addressed code style issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be working on this PR and applying a few modifications to avoid too much memory consumption.
foreach ($this->storage as $recording) { | ||
$storedRequest = Request::fromArray($recording['request']); | ||
if ($storedRequest->matches($request, $this->getRequestMatchers())) { | ||
return Response::fromArray($recording['response']); | ||
} | ||
$storedRequestData[] = $storedRequest->toArray(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$storedRequestData could grow very large (in particular if we are dealing with many file uploads).
This could lead to memory issues, and the "Stored requests:" part of the exception will very likely be not that readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experience having values from the stored requests and the used matchers is key to determining why none of the stored requests apply when you think they should. This includes the url, headers etc.
I agree about problems regarding readability and file uploads. Perhaps some fields could be excluded or truncated.
. 'Please see http://php-vcr.github.io/documentation/configuration/#record-modes.', | ||
0, | ||
$e | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal: playback
could keep returning null
and Videorecorder::handleRequest should check for a null return value and throw the exception (with a limited error message).
It may be tricky to determine why a recorded response is not used as no
information about how the actual request compares to the recorded once.
This change changes the behaviour of the playback method to throw an
exception is a match is not found. The message contains information
about the involved requests.
If PHP-VCR is also configured not to allow recording of new requests
then the new exception is chained to the existing one.
Additional information related to request matching is added to the new
root exception.
This relates to the original problem in #94.
The resulting output will look something like this: