search

westfieldlabs / apivore

Tests your rails API against its Swagger description of end-points, models, and query parameters.
Apache License 2.0
213 stars 66 forks source link

[Breaking change] Apivore v1 #40

Closed gingermusketeer closed 9 years ago

gingermusketeer commented 9 years ago

This is a work in progress. Need to be cleaned up a bit. Feedback welcome

hornc commented 9 years ago

Hi @gingermusketeer , I am concerned that these changes look like they are just testing the contents of the swagger documentation, when the original intent of this tool was to generate and run tests against the actual api using the swagger documentation as the source of truth on how the api is meant to behave.

hornc commented 9 years ago

To clarify my comment above, I'm basing my concern on the updated request_spec.rb test that now does not appear to make any requests to the API, where previously it made one request for each path, it just makes assertions against the swagger doc.

It seems to me that there we want a better way to test our swagger documentation? Apivore originally worked with the assumption that the swagger documentation was generally correct, and only provided a single basic schema validation pass, then used that to test that the API responded as documented.

If there was a mis-match, apivore would try to state what the mismatch was, but the concept of 'line numbers' isn't really applicable in many cases if a whole route or response does not exist in the api. It is intended to run as a request spec.

Maybe there is a misunderstanding of what apivore is trying to do, and a gap in what else we need?

gingermusketeer commented 9 years ago

@hornc It still interacts with the actual api. See https://github.com/westfieldlabs/apivore/pull/40/files#diff-62a36f1ebf6812e74bd31f41f5b85b85R21

hornc commented 9 years ago

Ah, thanks @gingermusketeer, I see that now. I was thrown by the expectation verb document I accept that it is hard to think of a word that covers it all. I originally went with validates as is was sufficiently vague to cover everything with "validates that the swagger correctly documents the api"

gingermusketeer commented 9 years ago

@hornc This PR started out as a proof of concept. I was not sure if it was possible to do this before i started. Happy to change any wording

gingermusketeer commented 9 years ago

@hornc All the tests are passing but i can't see the status on this branch :( not sure if that is something you can fix.

I have changed deal service to use this api. You can see it in action here https://github.com/westfield/deal_service/pull/84

jgalilee commented 9 years ago

:+1: :100: :whale2:

This looks fantastic to me. The refactor into an object will make debugging far easier.

adamcohen commented 9 years ago

LGTM great work @gingermusketeer !

jgalilee commented 9 years ago

:100: :whale2: :+1:

leondewey commented 9 years ago

@gingermusketeer @hornc Im worried about this PR. I thought it was a syntax change not a relaxing/change in philosophy of the way we are testing.

Can the three of us catch up before this get merged or has any more work done on it.

hornc commented 9 years ago

The method validate_all_paths() does not give accurate failure messages, and I think perhaps it is mis-named? validate() checks the swagger and makes requests to the API, I suggested the name validate_all_paths() because I thought it did a similar thing, because now I see that it only checks that all documented paths have been tested.

3) API testing scenarios unexpected http response fails
     Failure/Error: expect(subject).to validate_all_paths
       get /services.json is undocumented for response codes ["222", ["#", "paths", "/services.json", "get", "responses", "222", "schema"]]
     # ./spec/data/example_specs.rb:26:in `block (3 levels) in <top (required)>'

In my test swagger there was a documented path for /services.json response code 222. I think the error was that there was no explicit test for it in prior tests.

The documented path for get /services.json is untested for response codes ["222", ["#", "paths", "/services.json", "get", "responses", "222", "schema"]] is more accurate? I could be wrong because this method confused me at first.

gingermusketeer commented 9 years ago

Yep. That is probably a better message. I think the main thing with this PR is that we want to do a good job with the api so people dont need to change it too often. Improving error messages can be done really easily post merge. If you can think of a better name that would be great. I will look at making these changes tomorrow.

gingermusketeer commented 9 years ago

@hornc I have tweaked the message when paths are untested.

I can't think of a better name for the validate all method...

hornc commented 9 years ago

re. validate_all_paths naming, after some more though and discussion with @leondewey, I think it can stay like that, and we can add some code to attempt to validate the not -specifically tested paths against the API, if it can (no substitutions etc required), and then spit out a report about what could not be tested, and possibly generate an rspec template from the swagger that can be copied back into the tests?