Bring back assert_serializer

[maurogeorge]

Hi guys,

closes #1011

The assert_serializer test helper was removed at some point.

This patch brings back the assert_serializer test helper. This is the last
revision1 that has the helper. The original helper was used as base.

[bf4]

nice

[trek]

@maurogeorge looks like this needs a rebase.

[maurogeorge]

@trek rebased. Thanks for poiting

[joaomdmoura]

@maurogeorge could you move this to an article on our new docs and maybe but a reference here in the README?
Maybe into the "How to" section explaining hot to assert serializer.

[joaomdmoura]

@maurogeorge this looks nice, thank you for your contribution 😄 we made some comments on it, can you check it?

[maurogeorge]

Hi @joaomdmoura thanks for the review. Updated with the changes 😄

[maurogeorge]

I think that the broken test on the AppVeyor is not related with this PR, this test is broken on the master too.

[bf4]

I'd rather put something like this in active_model_serializers/test.rb, no? like

• https://github.com/rails/rails/blob/master/activejob/lib/active_job/test_case.rb

require 'active_support/test_case'

module ActiveJob
  class TestCase < ActiveSupport::TestCase
    include ActiveJob::TestHelper
  end
end

• https://github.com/rails/rails/blob/3e36db4406beea32772b1db1e9a16cc1e8aea14c/activejob/lib/active_job.rb#L35-L36

module ActiveJob
  extend ActiveSupport::Autoload

  autoload :TestCase
  autoload :TestHelper
end

then

class ActiveJobTestCaseTest < ActiveJob::TestCase

• https://github.com/rails/rails/blob/master/activejob/lib/active_job/test_helper.rb

[maurogeorge]

Thanks for the review ❤️

I try something in this line, but got no sucess 😞 since Active Job defines the ActiveJob::TestCase in our case we do not want to define our TestCase class but mixes the helper in the ActionController::TestCase.

I followed the same idea we have in the in the ActionController::TestCase. I renamed/move the assertions module to a better place.

Let me know what you think

[maurogeorge]

@joaomdmoura @bf4 sorry for the delay I was involved with other things 😞

I update the code following the @bf4 last comments. I waiting for your review guys ❤️

[trek]

Appears to be more merge conflicts :-/

[maurogeorge]

@trek I solved the conflicts, thanks for the report.

[bf4]

This shouldn't be eager-loaded here as it's a test-only concern

[maurogeorge]

Thanks for the feedback.

I think if we try to autoload ActiveModel::Serializer::Assertions, this will be loaded when we mix this on the ActionController::TestCase, in this same file, so this wil be loaded too soon.
Do you have some idea how we can lazy load this?

[bf4]

fwiw, here's some RSpec stuff I wrote for rc1 #878 (comment) that I've been meaning to work into ams in #967

[maurogeorge]

@bf4 thanks for the review 💯 , I answered each directly.

Pretty cool the RSpec stuff.

In my opinion I think we can try to generate test helpers like this and this this way we do not depend on RSpec, and the people that are using RSpec can mix this module and use with RSpec too, like we do for example with the ActiveSupport::Testing::TimeHelpers.

What you think?

[bf4]

Reading comments via email now (no code) but sounds good

B mobile phone

On Oct 15, 2015, at 4:50 PM, Mauro George [email protected] wrote:

@bf4 thanks for the review , I answered each directly.

Pretty cool the RSpec stuff.

In my opinion I think we can try to generate test helpers like this and this this way we do not depend on RSpec, and the people that are using RSpec can mix this module and use with RSpec too, like we do for example with the ActiveSupport::Testing::TimeHelpers.

What you think?

—
Reply to this email directly or view it on GitHub.

[maurogeorge]

@bf4 @joaomdmoura @beauby I update the code could you guys please review? Thanks

[maurogeorge]

@bf4 @joaomdmoura @beauby I updated to keep all tests green. Thanks

[trek]

@bf4 I don't suppose this could get merged or closed? Poor @maurogeorge has to keep rebasing to keep it current with master.

[beauby]

This looks good to me. @bf4 @joaomdmoura?

[bf4]

So, people using RSpec would need to define assert(expected, actual) and any library not using ActionController::TestCase would also need to include ActiveModelSerializers::Test::Serializer (I forget if RSpec-Rails uses this)

something like:

module AMSAssertSerializer
  extend ActiveSupport::Concern
  included do
    include ActiveModelSerializers::Test::Serializer
  end
  def assert(expected, actual)
    expect(actual).to eq(expected)
  end
end
RSpec.configuration.include AMSAssertSerializer, type: :controller

[maurogeorge]

Thanks for the review.

I don't think we need to care with RSpec and other libs at the moment, Action Mailer uses Minitest methods and Active Job too.

I think we can follow the same rationale, we are providing Minitest test helpers.

[bf4]

Yeah, I know. :)

[bf4]

Looking good. I had a few questions above and I'm ok to merge.

@trek @maurogeorge is this something you'd like to use? Just confirming because I kind of agree with @bolshakov that it doesn't seem very useful. But, given that I'm thinking/saying this 4 months after the PR was opened, if people want to use it, I'll merge it.

[bf4]

IMHO, this should only be loaded in the test environment. Sell me on always loading it, and why it won't cause issues in production (including the railtie ActionController::TestCase.send(:include, ActiveModelSerializers::Test::Serializer)

• ref to previous comment lost in the diffs

[bf4]

Oh, and #1270 (comment)

[maurogeorge]

@trek @bf4 @beauby I updated the code, based on the last comments.

The Travis is broken, but I don't think this is related with this patch, It seems that these tests already be broken on master.

[bf4]

@maurogeorge I have a PR to fix master, but wanted some eyes on it #1384

You're right, not related

[trek]

Yes, this is something we'd use in unit tests for controllers. We have separate unit tests for the serializers themselves (so ensure they match the a described JSON Schema), but are also currently reusing this strategy for controller tests, which seems a bit odd.

[bf4]

@trek so you're saying you used but don't think it's a good idea? (ref what's I've done in the past)

[maurogeorge]

Closing this PR, since the code was moved to the new one #1390

[trek]

@bf4 no, I'm saying schema validation in both unit tests for a serializer and as part of a unit test of a controller means you get twice the failures.

[bf4]

@trek so you basically are saying if someone is using assert_json_schema, there's no need for assert_serializer? Makes sense. Do you think both option should exist i.e. be supported by AMS and its people?

[trek]

@bf4 I think which one you use will depend on your testing strategy. We've moved on from wanting assert_serializer just due to the need to get coding. But, now we get multiple test failures if someone changes the schema accidentally.