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 request: indicate why cassette does not have necessary response #94

Open
tylercollier opened this issue Jan 19, 2015 · 16 comments
Labels
Feature New feature or request

Comments

@tylercollier
Copy link

I got this message:

LogicException: Invalid http request. The cassette inserted did not have the necessary response. If you want to send a request anyway, make sure your mode is set to new_episodes.

Another developer had modified our code, such that the body that was used for an http request was different. He did not modify the cassette, which he should have.

My issue is with the message. How about something like "The request body does not match a previous request"? Or, as I was digging through the code, the failure was not actually on the body, but the Content-Length. Same difference... I think a message saying "The request header ("Content-Length: 1567") did not match an existing request" would be more helpful.

This took me an hour to figure out myself, because I had to dig through the PHP-VCR source code. A more helpful message would've helped me understand the issue immediately.

@tylercollier
Copy link
Author

As follow-up motivation, I have another developer running tests, and they are failing with that same message. Helping him debug this remotely is going to be a pain. The fact that he's a rookie is not the fault of php-vcr, but why doesn't it just say

Invalid http request because [REASON]

?

Also, I just noticed that the message says "The cassette inserted did not have the necessary response", but should the last word be "request"?

@adri
Copy link
Contributor

adri commented Jan 22, 2015

That you very much Tyler. I think so too, there should be a way to easier find out what is happening (#36 is related to this as well).

"The request header ("Content-Length: 1567") did not match an existing request": How can we find out which parts of a request did not match? Not sure if it is possible. What might work is a list of all requests in a cassette with information why it didn't match.

I'm thinking about using #86 to provide a debug mode which then logs information somewhere. I wonder where that ideally should be. It could be in the exception as part of the message:

The request body does not match a previously recorded request. 
[... more information about the request ...] Recorded requests in the cassette [...cassette path...]:
 * GET /example, didn't match the header
 * GET /example2, didn't match the body

What do you think?

@tylercollier
Copy link
Author

It's easy to find out which parts didn't match a given request, right? It has to be; that's how VCR works. I assume what you mean is, how to convey that information to the user without overwhelming them, if there are multiple requests in a cassette? One idea is to only show what part didn't match if there's a single request in the given cassette. That works well for me, because I only ever have a single request per cassette. Even then, if it didn't match the header, I'd like to see why. Show me the current request and the one from the cassette. Any example where this works in (non-PHP) VCR is here: you can set the debug logger during bootstrapping.

But for the situation where there are multiple requests in a cassette, if you include a debug mode, there could still be 100 requests in a cassette. That'd be too many * GET /example, didn't match the header messages! I don't know how common this is though to have 100. If it's between 1 to 10 that might be fine. I suppose if you include a debug mode, then people can make the choice for themselves, which is a good starting point IMO.

I will add some thoughts directly to #86.

Also, did you see this question of mine?

Also, I just noticed that the message says "The cassette inserted did not have the necessary response", but should the last word be "request"?

(Because it matches on requests, not responses, doesn't it?) I want to know if I'm right to make sure I'm understanding PHP-VCR's intent!

@adri
Copy link
Contributor

adri commented Jan 22, 2015

Ah yes, some projects have many requests in one cassette. But when using different cassettes for each test (which is the preferred way) I can imagine very detailed debugging info. Thank you for your link to the VCR examples!

Answering your question: You are absolutely right. The last word should be "request" or at least "The cassette inserted did not have the necessary response for the request". But I prefer your proposal or something like:

The request does not match a previously recorded request.

@aaa2000
Copy link
Contributor

aaa2000 commented Jan 24, 2015

Concerning the debug mode, if there are many requests records in the cassette, we could maybe detect the recorded request which looks like the most to the request via levenshtein or similar_text PHP function. Then, tell which part (header, body...) doesn't match or use a diff.

@adri adri added the Feature New feature or request label Jan 24, 2015
adri added a commit that referenced this issue Jan 24, 2015
@tylercollier
Copy link
Author

@adri Your new text is improved. I think it'd be clearer to change the word "a" to "any":

The request does not match any previously recorded request...

@aaa2000 That's a good idea. Deciding which match is "closest" is non-trivial. I think you could write some code that got you 95% of the way there. Then make sure the message mentions that the current request differs from PHP-VCR's best guess as to a previously recorded transaction. This will still be better than it is today, where you have to do the guesswork and comparing yourself.

@tylercollier
Copy link
Author

@aaa2000, are you going to do this? Otherwise I'll take a stab.

@aaa2000
Copy link
Contributor

aaa2000 commented Feb 17, 2015

@tylercollier No, I didn't try to implement it

@tylercollier
Copy link
Author

Ok, I'm excited to give it a shot. I'll do that by the end of March 1st, MST.

@adri
Copy link
Contributor

adri commented Mar 8, 2015

I'm trying to find out what output is most helpful for users. From my point of view the following data might be interesting (inspired by vcr/vcr#478):

  • the path of the current cassette
  • the used request matchers
  • the record mode
  • which of the requests was the closest match (if there is one, see below)
  • which field(s) didn't match
  • the content of the fields that didn't match (maybe even a diff)

I like the idea of detecting the best matching request. The only drawback is that there could be a case when all requests match equally well. Then choosing a single closest match is not particularly helpful.

As far as I see, Tylers implementation gets already a lot of that, it looks a bit like this (please correct me if I'm wrong, Tyler).

The closest match was request 1 of 3.

   Stored request: Host: example.com
  Current request: Host: exmpl.com

@tylercollier The reason why I bring this up is because I have no clear idea how this should look like in the end. I would like to help you better, but first I need an idea of what the result is going to be.

@adri
Copy link
Contributor

adri commented Mar 8, 2015

In general I think it would be handy to have a logger like vcr (ruby) has. Not only for explaining mismatches but also for learning about how php-vcr reacts to the system under test.

The question is to me how much information there has/should be in the exception message vs. in the log output.

@tylercollier
Copy link
Author

Those are good ideas. You are correct with the example output you gave.

You're right that there could be a case when all the requests match equally well. This "best match" idea is not a trivial task, so we want to be clear to the user that we have a best match, and not the match. I will write up whatever algorithm we end up using in the documentation (by the way, I still request feedback on my "best match" strategy, see here). Eventually, we could have multiple strategies and allow a user to configure one to use, or allow them to configure multiple as well as when to use a given strategy. But let's do this the simple way first :-)

I use (Ruby's) VCR myself. Personally, I find the mismatch explanation (bullets 4-6) more helpful than the long message it shows. But their message (aka bullets 1-3) is clear/useful, so having both is best. Someday I plan to port the mismatch explanation to Ruby VCR. It at least has the debug logger, though its not perfect either.

Timewise, I wanted to work on this today, so I'm not sure if I can get to bullets 1 and 2 (we already have 3-6).

I'm thinking (1) will be straightforward. IIRC PHP-VCR does not allow nested cassettes. For (2), it could be helpful to mention all used request matchers; it's likely only a subset would show up in the mismatch explanation.

@adri
Copy link
Contributor

adri commented Mar 8, 2015

I think the "best match" can be as simple as selecting the request with the minimum number of mismatches.

What is your opinion of the following exception message draft?

A request does not match any previously recorded request and the 'mode' is set to 'none'. 
If you want to send the request anyway, make sure your 'mode' is set to 'new_episodes'.

  POST https://example.com/user/login
  Host: example.com
  User-Agent: Guzzle/5.2.0 curl/7.37.1 PHP/5.6.0
  {"username": "test", "password": "test"}

PHP-VCR is using the cassette "tests/fixtures/login-with-correct-credentials.yml" and 
matches on host, url, headers, body and query_string. 

The closest match was request 2 of 3 in the cassette:

  POST https://example.com/user/login
  - User-Agent: Guzzle/5.2.0 curl/7.37.1 PHP/5.5.0
  + User-Agent: Guzzle/5.2.0 curl/7.37.1 PHP/5.6.0

@tylercollier
Copy link
Author

I like it. Perhaps a sentence before the -/+ mentioning what they represent, i.e. current request and stored request.

One thing to be careful of is assuming the values are not too long. Following Ruby VCR's lead, you might want to truncate to some line length by default. It would be nice if the truncation length were configurable.

Your strategy to find the best match does not include a tie-breaker. I think it's likely that two stored requests would differ on the same Field (to keep using that term from #111) e.g. url. I like aaa2000's idea of the levenshtein function as a tie breaker.

The message format can be independent of the algorithm used to determine the best match.

@adri
Copy link
Contributor

adri commented Mar 27, 2015

Here is a project which does a diff of http. How do you like the format?
https://github.com/jgrahamc/httpdiff

@tylercollier
Copy link
Author

It's cool that httpdiff has colored output, although the default looks bad to me.

I prefer the standard diff format. Isn't that what phpunit uses by default for diff-type tests? I'm not sure how easy it would be to get diff coloring into phpunit.

I'm sorry I haven't updated you. I haven't looked at this since the last time I posted except to read your comments. I likely will not have time to work on it until April 11/12. The cool thing about making this feature in a robust manner is that it should make future changes (such as formatting with httpdiff, etc) much easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants