-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
5cb387f
to
5024b87
Compare
@maurogeorge I made a bunch of changes in small commits, mostly explained in the commit message. I look forward to hearing your thoughts. |
5024b87
to
a9c88b6
Compare
assert_match 'assert_serializer only accepts a String, Symbol, Regexp, ActiveModel::Serializer, or nil', e.message | ||
end | ||
|
||
def test_does_not_overwrite_notification_subscriptions |
There was a problem hiding this comment.
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 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
LGTM 👍 |
8ca809b
to
11fda59
Compare
@bf4 great job, I added some comments. |
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 |
5778719
to
c962b9a
Compare
|
||
included do | ||
setup :setup_serialization_subscriptions | ||
teardown :teardown_serialization_subscriptions |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
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.teardown :teardown_serialization_subscriptions
—
Reply to this email directly or view it on GitHub.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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 |
c962b9a
to
6508637
Compare
Should be ready to go now |
|
||
def initialize | ||
@serializers = Set.new | ||
self._subscribers = [] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed!
There was a problem hiding this comment.
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...
@_subscribers = []
@_subscribers << ActiveSupport::Notifications.subscribe("render_template.action_view") do |_name, _start, _finish, _id, payload|
#....
@_subscribers.each do |subscriber|
ActiveSupport::Notifications.unsubscribe(subscriber)
end
There was a problem hiding this comment.
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.
@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.
6508637
to
63a47f5
Compare
Updated |
63a47f5
to
3bc653f
Compare
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
3bc653f
to
f5e2b99
Compare
|
||
# For Rails4.0 | ||
def render_some_text | ||
Rails.version > '4.1' ? render(plain: 'ok') : render(text: 'ok') |
There was a problem hiding this comment.
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 LGTM |
Bring back assert_serializer for controller testing
Thanks for the merge ❤️ |
Can someone provide the steps I need to take in order to make the |
(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 lastrevision 2 that has the helper. The original helper was used as base.
assert_template
but we need this on the tests ofthe helper.
rails-controller-testing
to keep support onassert_template
.