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

Support hash as first argument in `assert_difference`. #31600

Merged
merged 3 commits into from Jan 18, 2018

Conversation

Projects
None yet
6 participants
@julien-meichelbeck
Contributor

julien-meichelbeck commented Dec 30, 2017

Summary

assert_difference already supports multiple expressions but only when the expected difference is the same for all of them (assert_difference [ 'Article.count', 'Post.count' ], 2).

This pull-request allows to specify multiple numeric differences in the same assertion which is also very convenient to avoid deep block nesting.

assert_difference 'Article.count' => 1, 'Notification.count' => 2 do
  # post :create, params: { article: {...} }
end

Other Information

Implementation: I also removed the each_with_indexwhich seemed redundant with the usage of the zip method.

@rails-bot

This comment has been minimized.

rails-bot commented Dec 30, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@sgrif

This comment has been minimized.

Member

sgrif commented Jan 12, 2018

/cc @rafaelfranca @dhh I like this change, but I think this needs sign-off from a core team member

r? @rafaelfranca

@rails-bot rails-bot assigned rafaelfranca and unassigned sgrif Jan 12, 2018

@dhh

This comment has been minimized.

Member

dhh commented Jan 13, 2018

I like the option to be able to declare multiple tests, but I’m not so keen on encouraging the string-eval approach further. I’d say that’s a deprecated form. If we have a way of letting the expression take multiple procs, then that’s probably the way to go instead.

@julien-meichelbeck

This comment has been minimized.

Contributor

julien-meichelbeck commented Jan 13, 2018

I like the option to be able to declare multiple tests, but I’m not so keen on encouraging the string-eval approach further. I’d say that’s a deprecated form. If we have a way of letting the expression take multiple procs, then that’s probably the way to go instead.

It also works with multiple procs (I just pushed a testcase).

@dhh Regarding the deprecation of the string-eval approach, I couldn't agree more and I was planning on submitting a third approach in a future pull-request which would be ActiveRecord_Relation.

99% of the time when I'm using assert_difference it's on models, why should I bother with string-eval and procs when I could just write this:

# Array
assert_difference [Article, Post.published], 2 do
end
# Hash
assert_difference Article => 1, Post.published => 2, Notification.sent => 4 do
end

Anyway, I'm going off-topic, just wanted to know if you guys like this idea.

@dhh

This comment has been minimized.

Member

dhh commented Jan 13, 2018

Looks good with the Proc approach. Let’s highlight that in the change log and the docs. Not loving the straight scope approach. Too much has to be inferred from that re: count. Thanks for working on this!

@eugeneius

This comment has been minimized.

Member

eugeneius commented Jan 13, 2018

Passing a custom error message seems to be broken here - I would expect this to work:

assert_difference({ ->{ Article.count } => 1, ->{ Post.count } => 2 }, "an article and two posts should be created") do
  # ...
end

julien-meichelbeck added some commits Dec 30, 2017

Support hash as first argument for `assert_difference`.
This allows to specify multiple numeric differences in the same assertion.
Example:

    assert_difference 'Article.count' => 1, 'Notification.count' => 2 do
      # post :create, params: { article: {...} }
    end
@julien-meichelbeck

This comment has been minimized.

Contributor

julien-meichelbeck commented Jan 15, 2018

@eugeneius Thanks, done !

@eugeneius

This comment has been minimized.

Member

eugeneius commented Jan 15, 2018

Nice one 👍

@@ -1,3 +1,9 @@
* Support hash as first argument in `assert_difference`. This allows to specify multiple
numeric differences in the same assertion.
`assert_difference ->{ Article.count } => 1, ->{ Post.count } => 2`

This comment has been minimized.

@eugeneius

eugeneius Jan 15, 2018

Member

Can you give this code example its own paragraph and indent it with four spaces, instead of wrapping it in backticks?

http://guides.rubyonrails.org/contributing_to_ruby_on_rails.html#updating-the-changelog

This comment has been minimized.

@julien-meichelbeck

@rafaelfranca rafaelfranca merged commit e0f0d71 into rails:master Jan 18, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
codeclimate All good!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment