Bring back assert_serializer for controller testing #1390

bf4 · 2015-12-22T16:30:55Z

(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 · 2015-12-23T05:13:01Z

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

bf4 · 2015-12-23T05:19:09Z

test/active_model_serializers/test/serializer_test.rb

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 )

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 · 2015-12-23T18:02:31Z

LGTM 👍

maurogeorge · 2015-12-23T20:51:28Z

@bf4 great job, I added some comments.

bf4 · 2015-12-31T19:13:54Z

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

Per rails-api#1390 (comment)

maurogeorge · 2016-01-05T19:05:28Z

lib/active_model_serializers/test/serializer.rb

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.

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 [email protected] 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.

Ref original comment 11fda59#commitcomment-15145827

Ok, I am good with that.

maurogeorge · 2016-01-05T19:09:56Z

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

Per rails-api#1390 (comment)

bf4 · 2016-01-13T05:26:57Z

Should be ready to go now

maurogeorge · 2016-01-13T21:07:44Z

lib/active_model_serializers/test/serializer.rb

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

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

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

maurogeorge · 2016-01-13T21:11:20Z

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

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. This is the last revision[2] that has the helper. The original helper was used as base. [1]: rails-api#596 [2]: https://github.com/rails-api/active_model_serializers/tree/610aeb2e9297fa31b8d561f0be9a4597f0258f8c - 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

In 0.9 (which this implementation is based on), the instrumentation was `!serialize.active_model_serializers`. rails-api#596 The '!' in the event name meant the event wasn't meant for production. https://github.com/rails/rails/pull/10446/files#r4075679 Since we intend the event for production and have a log subscriber, if we unsubscribe from `render.active_model_serializers`, we'll break other tests that are relying on that event being subscribed.

Per rails-api#1390 (comment)

bf4 · 2016-01-14T03:00:55Z

Updated

Per rails-api#1390 (comment)

Rails 5 removed this assertion after considering it not a good testing practice. rails/rails#18950 Rather that add a gem to our Rails 5 matrix to support it, the assertion is made that the template is rendering using active support notifications. Also, to clarify that the action 'render_template' is unrelated to the event name '!render_template.action_view', I renamed the actions so that would not look like event names.

This commit revises 0ce4ad3 `Remove unused/unusable unsubscribe since we don't want to unsubscribe` Looking at Rails implementation of assert_template which was likely the inspiration for assert_serializer: https://github.com/rails/rails-controller-testing/blob/f756b33c138c593eabe37f6085f8bac477b99bfe/lib/rails/controller/testing/template_assertions.rb Ref: - rails-api#596 - rails-api#620 - rails-api#616

Per rails-api#1390 (comment)

bf4 · 2016-01-14T03:47:55Z

test/active_model_serializers/test/serializer_test.rb

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

maurogeorge · 2016-01-14T20:23:30Z

@bf4 LGTM

Bring back assert_serializer for controller testing

maurogeorge · 2016-01-15T09:22:48Z

Thanks for the merge ❤️

dbwieland18 · 2016-04-18T14:23:01Z

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

remear · 2016-04-18T14:37:08Z

@dbwieland18 #1470

maurogeorge mentioned this pull request Dec 22, 2015

Bring back assert_serializer #1099

Closed

bf4 added C: Feature V: 0.10.x labels Dec 23, 2015

bf4 force-pushed the maurogeorge-patch-02 branch from 5cb387f to 5024b87 Compare December 23, 2015 05:10

bf4 force-pushed the maurogeorge-patch-02 branch from 5024b87 to a9c88b6 Compare December 23, 2015 05:14

bf4 reviewed Dec 23, 2015
View reviewed changes

bf4 force-pushed the maurogeorge-patch-02 branch from 8ca809b to 11fda59 Compare December 23, 2015 18:33

bf4 mentioned this pull request Jan 1, 2016

Add configurable serializers in each renderer rails/rails#21496

Closed

14 tasks

bf4 force-pushed the maurogeorge-patch-02 branch 2 times, most recently from 5778719 to c962b9a Compare January 4, 2016 05:36

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Jan 4, 2016

Remove outdated regression test

c962b9a

Per rails-api#1390 (comment)

maurogeorge reviewed Jan 5, 2016
View reviewed changes

bf4 force-pushed the maurogeorge-patch-02 branch from c962b9a to 6508637 Compare January 13, 2016 05:26

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Jan 13, 2016

Remove outdated regression test

6508637

Per rails-api#1390 (comment)

maurogeorge reviewed Jan 13, 2016
View reviewed changes

maurogeorge and others added 3 commits January 13, 2016 20:54

Be consisent in usage of ActiveSupport::Autoload

37a6d2b

Small perf, readability refactor to Test::Serializer

ef09c90

bf4 force-pushed the maurogeorge-patch-02 branch from 6508637 to 63a47f5 Compare January 14, 2016 03:00

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Jan 14, 2016

Remove outdated regression test

63a47f5

Per rails-api#1390 (comment)

bf4 force-pushed the maurogeorge-patch-02 branch from 63a47f5 to 3bc653f Compare January 14, 2016 03:07

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Jan 14, 2016

Remove outdated regression test

3bc653f

Per rails-api#1390 (comment)

bf4 added 5 commits January 13, 2016 21:47

Surface logging event name for re-use

28f314a

Remove duplicate documentation

e60937b

Remove outdated regression test

f5e2b99

Per rails-api#1390 (comment)

bf4 force-pushed the maurogeorge-patch-02 branch from 3bc653f to f5e2b99 Compare January 14, 2016 03:47

bf4 reviewed Jan 14, 2016
View reviewed changes

test/active_model_serializers/test/serializer_test.rb

Copy link

Member Author

bf4 Jan 14, 2016

Choose a reason for hiding this comment

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

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

bf4 added a commit that referenced this pull request Jan 14, 2016

Merge pull request #1390 from bf4/maurogeorge-patch-02

9aed6ac

Bring back assert_serializer for controller testing

bf4 merged commit 9aed6ac into rails-api:master Jan 14, 2016

bf4 deleted the maurogeorge-patch-02 branch January 14, 2016 23:54

Bring back assert_serializer for controller testing #1390

Bring back assert_serializer for controller testing #1390

Uh oh!

Conversation

bf4 commented Dec 22, 2015

Uh oh!

bf4 commented Dec 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joaomdmoura commented Dec 23, 2015

Uh oh!

maurogeorge commented Dec 23, 2015

Uh oh!

bf4 commented Dec 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maurogeorge commented Jan 5, 2016

Uh oh!

bf4 commented Jan 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maurogeorge commented Jan 13, 2016

Uh oh!

bf4 commented Jan 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maurogeorge commented Jan 14, 2016

Uh oh!

maurogeorge commented Jan 15, 2016

Uh oh!

dbwieland18 commented Apr 18, 2016

Uh oh!

remear commented Apr 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants