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

Adds :have_valid_schema RSpec matcher #1947

Closed
wants to merge 14 commits into from

Conversation

leonelgalan
Copy link
Contributor

@leonelgalan leonelgalan commented Oct 17, 2016

Purpose

Provides built-in matchers for RSpec users. Example:

require 'rails_helper'
require 'active_model_serializers/rspec'
ActiveModelSerializers.config.schema_path = 'spec/support/schemas'

RSpec.describe V1::PostsController do
  describe '#index' do
    before { get :index }

    describe 'response' do
      subject { response }
      it { is_expected.to have_valid_schema }
    end
  end
end

Changes

Adds modules:

  • ActiveModelSerializers::RSpecMatchers::Schema

Caveats

  • No tests, I'm still trying to figure out how to run rspec next to minitest and translate test/active_model_serializers/test/schema_test.rb to RSpec.
  • Couldn't use autoload, expected path would be /active_model_serializers/r_spec/matchers, as opposed to: /active_model_serializers/rspec/matchers. Doesn't apply anymore, see below.
  • Had to require 'minitest/autorun' on lib/active_model_serializers/test/schema.rb. Not anymore.

I could use some help solving this issues.

Related GitHub issues

@mention-bot
Copy link

@leonelgalan, thanks for your PR! By analyzing the history of the files in this pull request, we identified @maurogeorge, @bf4 and @beauby to be potential reviewers.

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Oct 17, 2016

Couldn't use autoload, expected path would be /active_model_serializers/r_spec/matchers, as opposed to: /active_model_serializers/rspec/matchers

is there a reason this is a bad thing?

I imagine to use this, we'd want to:

require 'active_model_serializers/rspec/matchers'

it's a common pattern in other gems to do it this way, I think.

unless you mean that you wanted to autoload the seperate files for rspec matchers.
autoload takes a second parameter that points to a file

end

class Base < ActiveModelSerializers::Test::Serializer::AssertSerializer
def subscribe
Copy link
Contributor

Choose a reason for hiding this comment

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

is this required for an rspec matcher?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by this?

  • I called the class Base for the lack of a better name. It's already inside ActiveModelSerializers::RSpec::Matchers::Serializer so it doesn't need any more specification.
  • I'm changing the subscribe method for my own, so it works with CollectionSerializers.

Copy link
Contributor

Choose a reason for hiding this comment

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

just, why does subscribe need to exist?

Are you saying that the built-in subscribe doesn't work for CollectionSerializer's?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't , the behavior without that change is the one described by @rafaelgonzalez on #1470 (comment). To be honest, I haven't tested this outside of my own matchers (directly on Minitest using builtin assertions). I will do a report back and write a failing test.

@leonelgalan
Copy link
Contributor Author

Not necessarily a bad thing, I just would have prefer not to change the existing files while adding the RSpec matchers.

I only wanted to use autoload, because that's how the the current files are loaded and I want to deviate the least from what's currently implemented. I will try that second param, but currently I simply require the file like you said.

@NullVoxPopuli
Copy link
Contributor

I read you. the problem with require with how you have it is that when lib/active_model_serializers/rspec/matchers.rb is required, the matcher files are also required. So for those that don't use rspec, or even during production use, that would lead to these rspec helpers erroring, because not all of the methods are defined. for the initial require of matchers, maybe you'll want do something like:

autoload RSpecMatchers, 'rspec/rspec_matchers' if defined? RSpec

I believe that will also protect against accidental requiring of the matchers in a non-test environment, provided that RSpec is only in the test env. But, def something you want to test out.

Also note that I changed the name of RSpec/Matchers to RSpecMatchers -- this is to prevent any module/class changes to rspec in the future.

So,

# lib/active_model_serializers/rspec/matchers.rb
module ActiveModelSerializers
  module RSpecMatchers
    # @api public
    # ...

I think that'll work, but anywho, give it a go?!

Thanks for your work on this! I'll def use these matchers in my own project :-)

@@ -15,6 +15,8 @@ module ActiveModelSerializers
autoload :JsonPointer
autoload :Deprecate

autoload :RSpecMatchers, 'active_model_serializers/rspec/matchers' if defined? RSpec
Copy link
Member

Choose a reason for hiding this comment

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

✂️

instructions should be put require 'active_model_serializers/rspec' in your rails_helper.rb just like capybara/rspec, it should be opt-in.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I will make the changes necessary for this instructions to work. Also, I'll add the changes to docs/howto/test.md

end

require_relative 'matchers/serializer'
require_relative 'matchers/schema'
Copy link
Member

Choose a reason for hiding this comment

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

require 'active_model_serializers/rspec/matcher/serializer' etc.

extend ActiveSupport::Concern

included do
RSpec::Matchers.define :have_valid_schema do |schema_path|
Copy link
Member

Choose a reason for hiding this comment

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

No need for a concern here. Just define the matcher. what's Base? should use a fully qualified path.

Copy link
Member

Choose a reason for hiding this comment

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

Could probably be ( just me typing here, expect errors)

      RSpec::Matchers.define :have_valid_schema do |api_schema|
          match do |request_or_response|
            schema_type = request_or_response.respond_to?(:env) && :request
            schema_type ||= request_or_response.respond_to?(:success?) &&  :response
            case schema_type
            when :request
                expect(request_or_response).to be_a_valid_api_request
                assert_request_schema api_schema
            when :response
                expect(request_or_response).to be_a_valid_api_response
                assert_response_schema api_schema
             else 
                 fail UnknownSchemaType.new("#{request_or_response} cannot be identified as a request or response")
             end
          end       
        end

alternatively, if we don't want a global matcher

included do
    require 'rspec/expectations'
    extend RSpec::Matchers::DSL

    matcher :have_valid_schema do |api_schema|
      match {|request_or_response|
      }
    failure_message {|request_or_response|
    failure_message_when_negated {|request_or_response|
    # etc.

Copy link
Member

Choose a reason for hiding this comment

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

see #1947 (comment) for some more ideas

It may be helpful for the matcher to rely on methods that can be overridden

In my experience, request and response matchers are different and it's very useful to validate headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will explore the suggested code and the one in the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Base was just poor naming, now that I move my "fix" to AssertSchema, I can simply initialize that class and use it as intended.

setup :setup_serialization_subscriptions
teardown :teardown_serialization_subscriptions

RSpec::Matchers.define :use_serializer do |expected|
Copy link
Member

Choose a reason for hiding this comment

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

Same comment except that this is really just a very strange thing to test in the first place. I personally don't get why people like it and understand why it's equivalent assert was removed from Rails. All you're asserting is that the serializer you want to use but aren't being explicit about is being looked up and used implicitly. If you care about which serializer is used enough to test it, just specify it in the render options in the controller.

My 2 💰

Also, no need for the setup and teardown

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, for me the JSON Schema is more important, because no matter what serializer is being used I simply want the response to be what I expect. That said, I was simply translating the current asserts to it's RSpec equivalents.

These setup and teardown set the subscriber needed to know what Serializer is being used on the render.

Copy link
Contributor

Choose a reason for hiding this comment

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

do we even need the subscriber in testing though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the subscriber to the::ActiveModelSerializers::Logging::RENDER_EVENT that allows us to see what serializer was used. At least that's how the current assertion works. That is, of course, if we want to keep this assertion/matcher. See https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model_serializers/test/serializer.rb#L67

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather just not include the use_serializer in this PR if even @leonelgalan doesn't use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll remove this. I was on a weekend vacation, but I'm ready to get this PR moving again.

@@ -1,3 +1,5 @@
require 'minitest/autorun'
Copy link
Member

Choose a reason for hiding this comment

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

This is a super bad idea. ✂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I should remove this and find another way to make it work? Never seen the ✂️ in a review before? I'll start by cutting this line and not pasting it anywhere else.

Copy link
Member

Choose a reason for hiding this comment

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

👅 Thanks for reading my comment in a positive light. Without knowing what problem you were working around, I wasn't sure what to write and didn't word it so well. autorun, specificaly, is almost certainly not what you want. no special require should be necessary here, except maybe ensure Minitest::Assertion is defined.

I usually use ✂️ and see it used to mean 'snip'.

@bf4
Copy link
Member

bf4 commented Oct 20, 2016

Here's the code I have:

# Usage:
#   let(:headers) { api_headers(3) }
#    RSpec.describe Api::V3::BaseController, :jsonapi, type: :request do
#      it "responds with 401 if user does not exist" do # etc.
#      it 'responds with a 404 when the JSON API Accept header is missing',          :show_exceptions, jsonapi: [:not_response, :not_request] do # etc.
#    end
module ApiTesting
  include ActiveModelSerializers::Test::Schema

  def api_headers(version)
    if version == 3
      {
        'Accept' => "application/vnd.api+json",
        'Content-Type' =>  'application/vnd.api+json'
      }
    else
      {
        'Accept' => "application/vnd.example.v#{version}",
        'Content-Type' =>  'application/json'
      }
    end.merge!(
      'HTTP_HOST' => 'api.example.com',
      'connection' => 'close'
    )
  end

  def request_payload
    request.env["action_dispatch.request.request_parameters"].dup
  end

  def json_response
    JSON.parse(response.body, symbolize_names: true)
  end

  def response_content_type
    response.headers['Content-Type']
  end

  def request_accepts
    request.headers['Accept']
  end

  def jsonapi_request?
    # request.format == :jsonapi # Doesn't work for some reason
    request.accepts.any?(&:jsonapi?) &&
      request_accepts.include?(jsonapi_content_type)
  end

  def jsonapi_response?
    return true if response.body.empty? && response_content_type.nil? && jsonapi_request?
    response_content_type == "#{jsonapi_content_type}; charset=utf-8"
  end

  # FIXME: bug in api schema that erroneously requires 'id' on POST create
  def assert_jsonapi_request!
    return unless jsonapi_request? && request_payload.present?
    if request.post?
      payload = request_payload
      payload['data']['id'] = 'create_does_not_require_id'
      assert_schema payload, api_schema
    else
      assert_request_schema api_schema
    end
  end

  def assert_jsonapi_response!
    return unless jsonapi_response? && response.body.present?
    assert_response_schema api_schema
  end

  def assert_not_jsonapi_response!
    return unless response.body.present?
    error = assert_raises Minitest::Assertion do
      assert_response_schema 'jsonapi.json'
    end
    assert_match(/failed schema/, error.message)
  end

  # from https://github.com/json-api/json-api/blob/c4df8dae8c8b1e97169b6d92193b62749240fbc5/schema
  def api_schema
    'jsonapi.json'
  end

  def jsonapi_content_type
    'application/vnd.api+json'
  end
end
RSpec.configure do |config|
  config.include ApiTesting, type: :request

  jsonapi_spec_missing_api_testing = ->(v) { !!v && !self.class.included_modules.include?(ApiTesting) } # rubocop:disable Style/DoubleNegation
  config.include ApiTesting, jsonapi: jsonapi_spec_missing_api_testing
  # When metadata has key `:jsonapi`,
  # perform validations according to its values.  Defaults to all validations.
  # @usage any of:
  #   `:jsonapi`,
  #   `jsonapi: true`
  run_all = ->(v) { v.nil? || v == true }
  hashify = ->(input) { input.is_a?(Hash) ? input : (input == true ? {} : input.to_h) } # rubocop:disable Style/NestedTernaryOperator
  includes = ->(input, value) { hashify.(input).key?(value) }
  excludes = ->(input, value) { Array(hashify.(input)[:except]).include?(value) }
  only = ->(input, value) { Array(hashify.(input)[:only]).include?(value) }

  # @usage any of:
  #   `:jsonapi`,
  #   `jsonapi: true`
  #   `jsonapi: :request`
  #   `jsonapi: [:request]`
  validate_request_schema = ->(v){ run_all.(v) || includes.(v, :request) }
  config.after jsonapi: validate_request_schema do
    next if request.nil?
    assert_jsonapi_request!
  end

  # @usage
  #   disable request validation with any of:
  #   `jsonapi: { except: :request }`
  #   `jsonapi: { except: [:request] }`
  validate_request_accepts_media_type = ->(v){ !excludes.(v, :request) }
  config.after jsonapi: validate_request_accepts_media_type do
    next if request.nil?
    expect(jsonapi_request?).to eq(true)
  end

  # @usage any of:
  #   `:jsonapi`
  #   `jsonapi: true`
  #   `jsonapi: :response`
  #   `jsonapi: [:response]`
  validate_response_schema = ->(v){ run_all.(v) || includes.(v, :response) }
  config.after jsonapi: validate_response_schema do
    next if response.nil?
    assert_jsonapi_response!
  end

  # @usage disable with response validation with any of:
  #   `jsonapi: { except: :response }`
  #   `jsonapi: { except: [:response] }`
  validate_response_content_type = ->(v){ !excludes.(v, :response) }
  config.after jsonapi: validate_response_content_type do
    next if response.nil?
    expect(jsonapi_response?).to eq(true)
  end

  # @usage expect non-JSON API repsonse
  #   `jsonapi: { except: :response }`
  #   `jsonapi: { except: [:response] }`
  assert_response_schema_not_jsonapi = ->(v){ excludes.(v, :response) }
  config.after jsonapi: assert_response_schema_not_jsonapi do
    next if response.nil?
    assert_not_jsonapi_response!
  end
end

rails (5.0.0.1) and active_model_serializers (0.10.2), not providing a
path for the schema results in: “No Schema file at
test/support/schemas//.json”, this patch allows the assertion to
retrieve the file successfully from test/support/schemas/posts/show.json
@leonelgalan leonelgalan changed the title Adds :use_serializer and :have_valid_schema RSpec matchers Adds :have_valid_schema RSpec matcher Oct 24, 2016
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

this is much simpler than before. Is there anyway you can add an rspec test to verify that it works?

@@ -105,6 +105,32 @@ default schema path in an initializer.
ActiveModelSerializers.config.schema_path = 'spec/support/schemas'
```

### RSpec test helpers
Copy link
Contributor

Choose a reason for hiding this comment

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

wooo docs!

Copy link
Contributor Author

@leonelgalan leonelgalan left a comment

Choose a reason for hiding this comment

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

Example of failing tests for 93de2c7 https://github.com/leonelgalan/active_model_serializer_example/blob/master/test/controllers/post_controller_test.rb

Both tests produce: No Schema file at test/support/schemas//.json

Somehow, it works on the current tests (I couldn't write a failing test for this).

I'm working on adding the specs for this PR.

Do we need to test behavior on both, or test the AssertSchema class
once?
It appears the option is passed to minutest runner:

```
…
invalid option: --color

minitest options:
    -h, --help                       Display this help.
…
```
@NullVoxPopuli
Copy link
Contributor

So close @leonelgalan!

@leonelgalan
Copy link
Contributor Author

Getting there, I'll fix Rails 4.1 Issues, should I rebase the PR to squash all this "fix" commits into the feature?

@NullVoxPopuli
Copy link
Contributor

don't worry about rebasing or squashing, we'll handle that (Github has a squash and merge button)

@NullVoxPopuli
Copy link
Contributor

WOAH! tests passing

@yijia-zhan
Copy link

This is awesome! 👍

@NullVoxPopuli
Copy link
Contributor

needs changelog entry.

@leonelgalan can you add that?

@@ -42,6 +42,7 @@ group :bench do
end

group :test do
gem 'rspec-rails'
Copy link
Member

Choose a reason for hiding this comment

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

Anything required to run tests should be in gemspec. I am also inclined to think this should be its own gem. I 'll help

Copy link
Member

Choose a reason for hiding this comment

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

If you wanna steal, here's what I do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll move it to the gemspec. Didn't understand your second comment, maybe is missing a link?

require 'rspec/core/rake_task'
RSpec::Core::RakeTask.new(:spec) do |t|
t.pattern = Dir.glob('spec/**/*_spec.rb')
end
Copy link
Member

Choose a reason for hiding this comment

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

Consider 'mrspec' :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave that up to you (rails-api members) to decide. For me adding mrspec it's just another layer on top of what already works.

@@ -60,11 +60,11 @@ def call
attr_reader :document_store

def controller_path
request.filtered_parameters[:controller]
request.filtered_parameters.with_indifferent_access[:controller]
Copy link
Member

Choose a reason for hiding this comment

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

Was this a problem? I use this extensively without

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #1947 (review)

I couldn't write a failing test (in the existing suite), but there is a sample app with a test failing.

require 'action_controller/test_case'
require 'active_model_serializers'

$LOAD_PATH.unshift File.expand_path('../../test', __FILE__)
Copy link
Member

Choose a reason for hiding this comment

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

Oy, we really need a dummy app

Copy link
Member

Choose a reason for hiding this comment

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

oh, this is so we can require the test helpers in the test dir...

end
end

describe ActiveModelSerializers::Test::SchemaTest::MyController, type: :controller do
Copy link
Member

Choose a reason for hiding this comment

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

Disable monkey patch mode please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

describe ActiveModelSerializers::Test::SchemaTest::MyController, type: :controller do
routes { Routes }

include ActiveModelSerializers::RSpecMatchers::Schema
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't need to include it manually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you want it that by including require 'active_model_serializers/rspec ActiveModelSerializers::RSpecMatchers::Schema is included on all controller and request specs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like this?
https://gist.github.com/leonelgalan/b63d031ece0f3989185c087be3cea285

api/lib/active_model_serializers/rspec.rb:

module ActiveModelSerializers
  # @api public
  # Container module for active_model_serializers specific matchers.
  module RSpecMatchers
    extend ActiveSupport::Autoload
    autoload :Schema, 'active_model_serializers/rspec_matchers/schema'
  end
end

RSpec.configure do |config|
  config.include ActiveModelSerializers::RSpecMatchers::Schema, type: :request
  config.include ActiveModelSerializers::RSpecMatchers::Schema, type: :controller
end

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but no need for the autoload, IMO

RSpec::Matchers.define :have_valid_schema do |schema_path, message|
match do
@matcher = ActiveModelSerializers::Test::Schema::AssertSchema.new(
schema_path || nil, request, response, message || nil
Copy link
Member

Choose a reason for hiding this comment

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

schema_path || nil, request, response, message || nil

weird looking...

(schema_path || nil), request, response, (message || nil)

?

I'm thinking schema_path should be chained

  chain :at_path do |schema_path|
    @schema_path = schema_path
  end

Simply doing `require 'active_model_serializers/rspec’` is enough to
load the schema matcher. Removes autoload as suggested by @bf4, uses
require instead.
module ActiveModelSerializers
module RSpecMatchers
module Schema
RSpec::Matchers.define :have_valid_schema do |message|
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the message expectation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's use RSpec default message handling instead. the only difference is that the custom message is not prepended to the original message, it substitutes it.

For example in ActiveModelSerializers::Test::SchemaTest::MyController test_that_raises_error_with_a_custom_message_with_a_invalid_schema

Instead of

oh boy the show is broken: #/name: failed schema #/properties/name: For 'properties/name', \"Name 1\... schema #/properties/description: For 'properties/description', \"Description 1\" is not a boolean.

we simply get the custom message:

oh boy the show is broken

Uses RSpec’s default instead: pass a message as a second param to `.to`
 to override the `failure_message`.
@leonelgalan
Copy link
Contributor Author

Hey guys, what's left to get this merged? The only thing that I see is #1947 (comment)

Copy link
Member

@bf4 bf4 left a comment

Choose a reason for hiding this comment

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

Leonelgalan, thanks for keeping on top of this. Sorry I'm being so tough on you, but this is something that can impose a burden on maintainers once included....

require 'active_model_serializers/rspec'

# spec/controller/posts_controller_spec.rb
describe PostsController do
Copy link
Member

Choose a reason for hiding this comment

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

should be RSpec.describe PostsController, type: :controller

Copy link
Member

Choose a reason for hiding this comment

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

But actually, we should give an example from a request spec, since controller specs aren't very useful and are going away. Some discussion in #1970 (comment)

RSpec.describe PostsController, type: :request do


end


# spec/controller/posts_controller_spec.rb
describe PostsController do
describe 'index' do
Copy link
Member

Choose a reason for hiding this comment

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

should be context 'GET /posts' do IMO, since that's more descriptive and works well with autodoc tools

and then, per https://github.com/rails-api/active_model_serializers/pull/1947/files#r96015833 get '/posts', {}, headers

describe 'index' do
it "should render right response" do
get :index
expect(response).to have_valid_schema
Copy link
Member

Choose a reason for hiding this comment

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

which schema is this using? I'd rather the example be

expect(request).to have_valid_schema(api_request_schema)
expect(response).to have_valid_schema(api_response_schema)

expect(request).to have_valid_schema(post_request_schema)
expect(response).to have_valid_schema(post_response_schema)

end
```

It's usage is on par with `assert_response_schema`. See [ActiveModelSerializers::Test::Schema](../../lib/active_model_serializers/test/schema.rb) and [ActiveModelSerializers::RSpecMatchers::Schema](../../lib/active_model_serializers/rspec_matchers/schema.rb)
Copy link
Member

Choose a reason for hiding this comment

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

typo: Its

Probably better, something like:

`expect(request).to have_valid_schema(path_to_schema)` can be understood as being translated to `assert_request_schema(path_to_schema)`. 
See [ActiveModelSerializers::Test::Schema](../../lib/active_model_serializers/test/schema.rb) and [ActiveModelSerializers::RSpecMatchers::Schema](../../lib/active_model_serializers/rspec_matchers/schema.rb) for documentation of the schema assertions.

require 'action_controller/test_case'
require 'active_model_serializers'

$LOAD_PATH.unshift File.expand_path('../../test', __FILE__)
Copy link
Member

Choose a reason for hiding this comment

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

oh, this is so we can require the test helpers in the test dir...

end
end

RSpec.describe ActiveModelSerializers::Test::SchemaTest::MyController, type: :controller do
Copy link
Member

Choose a reason for hiding this comment

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

needs request spec. see https://github.com/rails-api/active_model_serializers/pull/1947/files#r96015833

Also, the test app should be defined inside the RSpec block or namespace

this globally creates a ActiveModelSerializers::Test::SchemaTest::MyController

Might be a good case to start a dummy app like in #1414

@@ -18,6 +18,7 @@ Features:
- [#1889](https://github.com/rails-api/active_model_serializers/pull/1889) Support key transformation for Attributes adapter (@iancanderson, @danbee)
- [#1917](https://github.com/rails-api/active_model_serializers/pull/1917) Add `jsonapi_pagination_links_enabled` configuration option (@richmolj)
- [#1797](https://github.com/rails-api/active_model_serializers/pull/1797) Only include 'relationships' when sideloading (@richmolj)
- [#1947](https://github.com/rails-api/active_model_serializers/pull/1947) Adds :have_valid_schema RSpec matche (@leonelgalan)
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be moved up now that there have been releases.

@leonelgalan
Copy link
Contributor Author

@bf4 no worries I can only imagine how much work a gem of this size is. I'll work on the requested changes and post them soon.

@kurko
Copy link
Member

kurko commented Jan 13, 2017

I know I'm not as involved in this project anymore, but I'm 👎 on adding RSpec inside this gem. We'll then have to add TestUnit, maybe mocha, and so on. These things should live outside this gem, as independent gems.

A couple years ago there were talks about merging AMS into Rails5. Although that didn't happen for N reason, making it a one-size-fits all gem moves it even farther away from that, besides forcing maintainers to maintain yet another dependency. When RSpec (and other dependencies) goes v4, v5, AMS will evolve slower.

@bf4
Copy link
Member

bf4 commented Jan 31, 2017

@leonelgalan would you be interested in continuing this work in a new repository?

@leonelgalan
Copy link
Contributor Author

I'll give it a try. It's been a busy past weeks. But I've schedule some time later on the afternoon to tackle this:

  • Create rspec-active_model_serializers repository.
  • Move all bits in this PR to new repo.

@bf4
Copy link
Member

bf4 commented Jan 31, 2017

@leonelgalan I started one some time ago but never came back to it https://github.com/bf4/active_model_serializers-rspec I can move it over to rails-api if you wish or otherwise give you commit.

@leonelgalan
Copy link
Contributor Author

leonelgalan commented Jan 31, 2017

I pushed my first take minutes before your comment: https://github.com/leonelgalan/rspec-active_model_serializers. The repo name with the rspec- prefix, comes from every other gem I found.

I would like to keep doing some cleanup/troubleshooting on the night before we make a decision on how to move forward. I'm ok hosting it myself or moving it into the rails-api organization, once you guys 👍 the code.

I have one broken test and code that fixes it, but that code looks like a NOOP to me and shouldn't affect the outcome, please see leonelgalan/rspec-active_model_serializers#1 for details.

 class ProfileSerializer < ActiveModel::Serializer
    attributes :name, :description
 +
 +  def name
 +    object.name
 +  end
  end

TODO (For today)

  • Take ownership of the tests and rewrite them for RSpec. The currrent tests is a translation of the existing tests. Instead of it 'test_that_assert_with_a_valid_schema' maybe the test should read