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

Proposal: Use conventions to define the schema path #22

Open
maurogeorge opened this issue Oct 16, 2015 · 5 comments
Open

Proposal: Use conventions to define the schema path #22

maurogeorge opened this issue Oct 16, 2015 · 5 comments

Comments

@maurogeorge
Copy link

Hi,

Recently I make a proposal to create a test helper on the AMS, that follows the same idea, use a schema file to validate a API response. But in my case I am using conventions to load the schema file. Example

describe "GET /posts" do
  it "returns Posts" do
    get posts_path, format: :json

    expect(response.status).to eq 200
    expect(response).to match_response_schema
  end
end

I remove the param of the schema path, and assumes that the schema file lives in spec/support/api/schemas/posts/index.json since it is the posts#index tests and make the param optional. Makes sense to you guys?

A little doubt, Makes sense this be a matcher on the shoulda-matchers? This way we will need only a dependency and we can use this with the Minitest too. I think we can handle this as a Action Controller matcher. @mcmire makes sense?

Thanks guys

@seanpdoyle
Copy link
Collaborator

@maurogeorge Thanks for opening this ticket.

This approach is very interesting. I'm curious if this convention could be inferred outside a Rails app (Sinatra, Grape, or other Rack apps).

Would you be willing to flesh out the implementation you're considering?

@maurogeorge
Copy link
Author

@seanpdoyle Yeah I really don't know if this convention could be inferred outside of Rails. But in any case we can start doing this behavior only on Rails apps and in other Rack apps the user need to pass the params explicitly. What you think?

@seanpdoyle
Copy link
Collaborator

@maurogeorge That's true. I think it depends on how robust the implementation is, and the sorts of patterns consumers use for declaring their schema.

Would you be willing to work a PR with an initial implementation or some failing test cases?

@mcmire
Copy link

mcmire commented Oct 16, 2015

This approach sounds interesting, but to me it sounds a little magical. I tend to avoid magic as much as I can in my tests, and I really like the current approach that we're taking with this matcher because it's easy to see which schema file is going to be referenced. If the test fails in some manner and the test doesn't have some kind of hint as which file contains the error, then it's going to be difficult to debug the test, especially for people who are not familiar with this matcher and using it for the first time. (I like to make tests as easy to debug as possible too.)

As far as merging this in with shoulda-matchers, we intentionally decided to keep this separate as we didn't really know if it would fit in and we figured we'd have to play around with it first internally. We may decide to merge it in the future, but for now I think it's best if we keep it separate.

@maurogeorge
Copy link
Author

@seanpdoyle Yeah I can work on a PR, maybe in the end of the next week, but after the @mcmire comments do you belive that's make sense this PR or prefer keep the things the way it is?

@mcmire thanks for your feedback. I understand your point, make the things explicit has values too, it is about trade-offs, if the file is a param we can have a folder with all schemas or a confusing organization, following a convention we try to avoid this.
About the shoulda-matchers thanks for clarifying 😄

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

No branches or pull requests

3 participants