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

Bring back assert_serializer for controller testing #1390

Merged
merged 9 commits into from
Jan 14, 2016

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Dec 22, 2015

(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
Copy link
Member Author

bf4 commented Dec 23, 2015

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

@bf4 bf4 force-pushed the maurogeorge-patch-02 branch from 5024b87 to a9c88b6 Compare December 23, 2015 05:14
assert_match 'assert_serializer only accepts a String, Symbol, Regexp, ActiveModel::Serializer, or nil', e.message
end

def test_does_not_overwrite_notification_subscriptions
Copy link
Member Author

Choose a reason for hiding this comment

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

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 )

Copy link
Member Author

Choose a reason for hiding this comment

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

Compare

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
Copy link
Member

LGTM 👍

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

@bf4 great job, I added some comments.

@bf4
Copy link
Member Author

bf4 commented Dec 31, 2015

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

@bf4 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

included do
setup :setup_serialization_subscriptions
teardown :teardown_serialization_subscriptions
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ref original comment 11fda59#commitcomment-15145827

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I am good with that.

@maurogeorge
Copy link
Member

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 :shipit:

@bf4 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
@bf4
Copy link
Member Author

bf4 commented Jan 13, 2016

Should be ready to go now


def initialize
@serializers = Set.new
self._subscribers = []
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed!

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@maurogeorge
Copy link
Member

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

maurogeorge and others added 3 commits January 13, 2016 20:54
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.
@bf4 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
@bf4
Copy link
Member Author

bf4 commented Jan 14, 2016

Updated

@bf4 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
bf4 added 5 commits January 13, 2016 21:47
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
@bf4 bf4 force-pushed the maurogeorge-patch-02 branch from 3bc653f to f5e2b99 Compare January 14, 2016 03:47

# For Rails4.0
def render_some_text
Rails.version > '4.1' ? render(plain: 'ok') : render(text: 'ok')
Copy link
Member Author

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+)

@maurogeorge
Copy link
Member

@bf4 LGTM :shipit:

bf4 added a commit that referenced this pull request Jan 14, 2016
Bring back assert_serializer for controller testing
@bf4 bf4 merged commit 9aed6ac into rails-api:master Jan 14, 2016
@bf4 bf4 deleted the maurogeorge-patch-02 branch January 14, 2016 23:54
@maurogeorge
Copy link
Member

Thanks for the merge ❤️

@dbwieland18
Copy link

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

@remear
Copy link
Member

remear commented Apr 18, 2016

@dbwieland18 #1470

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants