Bring back assert_serializer for controller testing

[bf4]

(This PR includes and extends the work done by @maurogeorge in #1099)

The assert_serializer test helper was added in 0.9.0.apha1 1,
and was not included in 0.10.

This patch brings back the assert_serializer test helper from the last
revision 2 that has the helper. The original helper was used as base.

• Create the AssertSerializer
• Use the Test namespace
• Make the tests pass on the Rails master
  • Rails 5 does not include assert_template but we need this on the tests of
    the helper.
  • This add the rails-controller-testing to keep support on assert_template.
• Only load test helpers in the test environment

[bf4]

@maurogeorge I made a bunch of changes in small commits, mostly explained in the commit message. I look forward to hearing your thoughts.

[bf4]

The intent of this test has changed a bit, and it should probably be renamed. I'm not really sure why asserting a template was rendered would relate to 'not overwriting notification subscriptions'. The assert template test does use the !render_template.action_view' event and I wonder if it was just that the original implementation used the same setup/teardown names as the rails assertion. ( setup :setup_subscriptions teardown :teardown_subscriptions )

[bf4]

Compare

• Test::Unit assert_serializer implemented #596 which uses the same setup/teardown methods as the rails assertion to
• Avoid overwriting notifications for template assertions. #620 which fixed ActionController subscriptions overwritten. #616, that action controller subscriptions were being overwritten.

So, the render_a_template tests are really failing tests prior to #620 that showed that #620 works. I'm not sure they're useful as regression tests.

[joaomdmoura]

LGTM 👍

[maurogeorge]

@bf4 great job, I added some comments.

[bf4]

lulz, I forgot I did this in my Rails serializers PR

+    def assert_serializer!(serializer_name)
+      return true if has_serializer?(serializer_name)
+      raise ActionController::MissingSerializer, "There is no '#{serializer_name}' serializer.\n" <<
+            "Known serializers are #{_serializers.keys}"
+    end

[maurogeorge]

We really need this teardown ? If we need, I think we need to write a broken test to it, what behavior will be broken if we do not have this teardown.

[bf4]

I don't think it needs a test.

It just unsubscribes the subscriber created in the test...

See more in the commit d9eefdb

I can try to show what may break without it, but given that the teardown is copied from rails, and I understand the behavior with and without...

B mobile phone

On Jan 5, 2016, at 1:05 PM, Mauro George notifications@github.com wrote:

In lib/active_model_serializers/test/serializer.rb:

@@ -0,0 +1,125 @@
+require 'set'
+module ActiveModelSerializers

• module Test
• module Serializer

•  extend ActiveSupport::Concern
  

•  included do
  

•    setup :setup_serialization_subscriptions
  

•    teardown :teardown_serialization_subscriptions
  

  We really need this teardown ? If we need, I think we need to write a broken test to it, what behavior will be broken if we do not have this teardown.

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

[bf4]

Ref original comment 11fda59#commitcomment-15145827

[maurogeorge]

Ok, I am good with that.

[maurogeorge]

Hi @bf4 I bring back some comments, I guess the last time I made these comments on the commit, probably they are lost after some forced push.

Are only 2 small comments and

[bf4]

Should be ready to go now

[maurogeorge]

I think we can use @subscribers over self._subscribers, I don't think we need to use _ to declare visibility.

[beauby]

Agreed!

[bf4]

I never expected this to get so much discussion! I just cargo-culted it from Rails and...

https://github.com/rails/rails-controller-testing/blob/f756b33c138c593eabe37f6085f8bac477b99bfe/lib/rails/controller/testing/template_assertions.rb#L21-L23

          @_subscribers = []

          @_subscribers << ActiveSupport::Notifications.subscribe("render_template.action_view") do |_name, _start, _finish, _id, payload|
#....
          @_subscribers.each do |subscriber|
            ActiveSupport::Notifications.unsubscribe(subscriber)
          end

[beauby]

I think the point is not that much about the underscore, but more about ivar vs accessors.

[maurogeorge]

@bf4 sorry for that, I added 2 small comments. For me after that

[bf4]

Updated

[bf4]

womp womp (Really for Rails 5, but it works on 4.1+)

[maurogeorge]

@bf4 LGTM

[maurogeorge]

Thanks for the merge ❤️

[dbwieland18]

Can someone provide the steps I need to take in order to make the assert_serializer helper available in an rspec test?

[remear]

@dbwieland18 #1470