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

Wisetara/deprecate args active support test case#assert nothing raised for pr #23789

Merged
merged 3 commits into from Feb 23, 2016

Conversation

Projects
None yet
5 participants
@wisetara

wisetara commented Feb 20, 2016

Using arguments (to specify an exception error) with assert_nothing_raised is not recommended, as discussed with Rafael França on #23773.

A deprecation warning has been added to ActiveSupport::TestCase#assert_nothing_raised(*args), and all tests have been updated to use assert_nothing_raised without arguments.

@wisetara

This comment has been minimized.

Show comment
Hide comment
@wisetara

wisetara Feb 20, 2016

Well, there may be a bit too much warning! I see that now. But I don't believe sorting that out will fix the failure:
TemplateDigestorTest#test_logging_of_missing_template [/home/travis/build/rails/rails/actionview/test/template/digestor_test.rb:109]:
Expected /Couldn't\ find\ template\ for\ digesting:\ messages/something_missing/ to match "".
If someone looks at it, would you #1, agree that perhaps a documented deprecation warning above the method is best rather than an actual warning (or I could code it to only show when arguments are passed in, I suppose), and #2, any thoughts on that failure.

wisetara commented Feb 20, 2016

Well, there may be a bit too much warning! I see that now. But I don't believe sorting that out will fix the failure:
TemplateDigestorTest#test_logging_of_missing_template [/home/travis/build/rails/rails/actionview/test/template/digestor_test.rb:109]:
Expected /Couldn't\ find\ template\ for\ digesting:\ messages/something_missing/ to match "".
If someone looks at it, would you #1, agree that perhaps a documented deprecation warning above the method is best rather than an actual warning (or I could code it to only show when arguments are passed in, I suppose), and #2, any thoughts on that failure.

@prathamesh-sonpatki

This comment has been minimized.

Show comment
Hide comment
Member

prathamesh-sonpatki commented Feb 20, 2016

Show outdated Hide outdated activesupport/lib/active_support/test_case.rb
@@ -75,6 +75,11 @@ def test_order
# perform_service(param: 'no_exception')
# end
def assert_nothing_raised(*args)
ActiveSupport::Deprecation.warn(<<-MSG.squish)

This comment has been minimized.

@meinac

meinac Feb 20, 2016

Contributor

Maybe you have to check presence of arguments. Now it's giving deprecation error every time even if you don't send any parameter to it.

@meinac

meinac Feb 20, 2016

Contributor

Maybe you have to check presence of arguments. Now it's giving deprecation error every time even if you don't send any parameter to it.

@wisetara

This comment has been minimized.

Show comment
Hide comment
@wisetara

wisetara Feb 22, 2016

That's better! I hope this deprecation PR is helpful, @rafaelfranca! Please let me know if you'd like any additional changes.

wisetara commented Feb 22, 2016

That's better! I hope this deprecation PR is helpful, @rafaelfranca! Please let me know if you'd like any additional changes.

Show outdated Hide outdated activesupport/CHANGELOG.md
@@ -1,3 +1,13 @@
* Deprecate `(*args)` on `assert_nothing_raised`.
Using arguments (to specify an exception error) with

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 23, 2016

Member

Thank you for mentioning me but we usually don't do that.

What do you think about?

Deprecate arguments on `assert_nothing_raised`.

`assert_nothing_raised` were not asserting the arguments that were being passed
(usually an specific exception class) since it only yields the block. To not confuse the
users that the arguments have any meaning they are being deprecated.
@rafaelfranca

rafaelfranca Feb 23, 2016

Member

Thank you for mentioning me but we usually don't do that.

What do you think about?

Deprecate arguments on `assert_nothing_raised`.

`assert_nothing_raised` were not asserting the arguments that were being passed
(usually an specific exception class) since it only yields the block. To not confuse the
users that the arguments have any meaning they are being deprecated.
Show outdated Hide outdated activesupport/lib/active_support/test_case.rb
@@ -75,6 +75,9 @@ def test_order
# perform_service(param: 'no_exception')
# end
def assert_nothing_raised(*args)
ActiveSupport::Deprecation.warn("Passing arguments to

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 23, 2016

Member
if args.present?
  ActiveSupport::Deprecation.warn("Passing arguments to assert_nothing_raised is deprecated and will be removed in Rails 5.1.")
end
@rafaelfranca

rafaelfranca Feb 23, 2016

Member
if args.present?
  ActiveSupport::Deprecation.warn("Passing arguments to assert_nothing_raised is deprecated and will be removed in Rails 5.1.")
end
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 23, 2016

Member

Awesome work. I made some comment.

Member

rafaelfranca commented Feb 23, 2016

Awesome work. I made some comment.

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Feb 23, 2016

@wisetara

This comment has been minimized.

Show comment
Hide comment
@wisetara

wisetara Feb 23, 2016

Thank you for the comments and feedback. I've incorporated your if-else refactor on testcase.rb, and I modified the CHANGELOG.md. I've rebased, and I hope this is good, but if you'd like any other changes, please let me know. It feels good to contribute!

wisetara commented Feb 23, 2016

Thank you for the comments and feedback. I've incorporated your if-else refactor on testcase.rb, and I modified the CHANGELOG.md. I've rebased, and I hope this is good, but if you'd like any other changes, please let me know. It feels good to contribute!

rafaelfranca added a commit that referenced this pull request Feb 23, 2016

Merge pull request #23789 from wisetara/wisetara/deprecate-args-Activ…
…eSupport__TestCase#assert_nothing_raised-for-pr

Wisetara/deprecate args active support  test case#assert nothing raised for pr

@rafaelfranca rafaelfranca merged commit 3adc35a into rails:master Feb 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 23, 2016

Member

Thank you

Member

rafaelfranca commented Feb 23, 2016

Thank you

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