find_in_batches should not mutate its argument #13878
Conversation
|
Of course it should not. 👍 |
| @@ -153,6 +153,12 @@ def test_find_in_batches_should_not_ignore_the_default_scope_if_it_is_other_then | |||
| assert_equal special_posts_ids, posts.map(&:id) | |||
| end | |||
|
|
|||
| def test_find_in_batches_should_not_modify_passed_options | |||
| assert_nothing_raised do | |||
There was a problem hiding this comment.
We avoid to use assert_nothing_raised. Can we assert that a enumerable is returned?
There was a problem hiding this comment.
I completely understand why assert_nothing_raised should be used very sparingly, I've seen many cases where it shouldn't.
I don't believe this is one of them though.
Note that testing that an Enumerable is returned is wrong for two reasons.
- It doesn't actually work, because the argument is mutated only when consuming it!
- I believe it puts the focus on the wrong thing. This test is not about what is returned, but about the fact that it doesn't blow up when given a frozen argument. We know what should be the effect and what should be returned, that's already covered in other tests.
I probably should blog about this, but here are the conditions for usage of assert_nothing_raised:
- What is in the block is not present in any other test.
- The test is solely about the fact that something doesn't blow up; in particular there are no other assertions in the test.
Then, and only then, one should use assert_nothing_raised. This is the case here.
There was a problem hiding this comment.
I understand your reasons but assert_nothing_raised doesn't do anything. It can be used for documentation reasons but it doesn't assert any value. If an exception is raised, the test will fail with ou without assert_nothing_raised.
I'll accept it as it is since this is a minor thing.
ghost
assigned 
find_in_batches should not mutate its argument
In case this needs justification: http://blog.marc-andre.ca/2009/03/02/leave-my-options-alone/