-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Deprecate assert_template
and assigns()
.
#20138
Conversation
|
||
process :hello_world | ||
assert_template 'hello_world' | ||
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.
7bcd3f9
to
db2d0fe
Compare
assert_template
.assert_template
and assigns()
.
Done! Gem published at https://rubygems.org/gems/rails-controller-testing. |
47e55c9
to
8c6c96e
Compare
@@ -102,6 +102,11 @@ def reset_template_assertion | |||
# # assert that the "_customer" partial was rendered with a specific object | |||
# assert_template partial: '_customer', locals: { customer: @customer } | |||
def assert_template(options = {}, message = nil) | |||
ActiveSupport::Deprecation.warn( |
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 should raise a NoMethodError and point to the gem so we can remove all the implementation
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 thought we had to deprecate it first?
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.
If we are extracting a gem there is no need. The NoMethodError
message should tell about the gem and people can get it work adding the gem to their Gemfile.
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.
Do you want me to transfer the Gem to Rails? Although Rails isn't supporting these two methods, it seems weird that we're pointing to my personal repository.
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.
👍 transfer to Rails so I can give you commit access.
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.
Hmm I can't transfer without admin rights. Better to fork in this case?
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.
transfer to me so I can transfer to Rails.
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.
Thanks! I'll remove the method soon.
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.
Don't remove the methods, just change their implementation to:
def assert_template(*)
raise NoMethodError, "assert_template was extracted to a gem. To continue using it use the ..."
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.
Ok I'll remove the internal tests in this case.
86b91d7
to
eda79db
Compare
Updated but there is some problem on Travis that isn't related. |
Great work! Could you add a CHANGELOG entry? |
eda79db
to
28d34e2
Compare
O yeah forgot about it XD. Done 😄 |
@@ -1,3 +1,9 @@ | |||
* Deprecate `assigns` and `assert_template`. |
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 it is better to tell that these methods were extracted to the gem and point to it on the changelog.
28d34e2
to
ca83436
Compare
@@ -1,3 +1,10 @@ | |||
* Remove `assigns` and `assert_template`. Both methods have been extracted | |||
into a gem at https://github.com/rails/rails-controller-testing. |
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.
@rafaelfranca Is this ok? 😄
Deprecate `assert_template` and `assigns()`.
👍 great job! |
I first thought this was markdown so I updated the feature post spec (it's now improved but unrelated to the fix). Then I discovered that it's because InternalPostsController was using the default application layout instead of the blog layout. Easy fix, but what about the test? I first tried to use a controller test. However, asserting that a specific layout is used is deprecated: rails/rails#20138 Then I tried a feature spec. This works, but I'm still just testing things unrelated to what I care about. In particular, the main navigation (About Projects Music) exists in the application layout, but not in the blog layout. That works for now, but it still didn't feel like the right way to test if a post has the correct styles applied to it. I landed on actually asserting on the styles using Capybara's [matches_style] matcher. This requires that I use a non-headless browser to render the CSS styles of a page. [matches_style]: https://rubydoc.info/github/teamcapybara/capybara/master/Capybara%2FNode%2FMatchers:matches_style%3F
Related: #18950.
cc/ @dhh