Skip to content

Conversation

@joshuay03
Copy link
Contributor

@joshuay03 joshuay03 commented Jan 5, 2024

Actioning this comment.

I didn't end up opening an issue just for this, please let me know if I should!

@flavorjones flavorjones merged commit c3d2e04 into rails:main Jan 13, 2024
@joshuay03 joshuay03 deleted the raise-an-error-for-no-elements-with-block branch January 16, 2024 04:04
@tomhughes
Copy link

Is this really reasonable? We have a number of places where we use helper functions that contain code like this one:

assert_select "item", :count => traces.length do |items|
  traces.zip(items).each do |trace, item|
    assert_select item, "title", trace.name
    ...
  end
end

That helper is called from various tests and may be given an empty list of traces or a list that has some entries and until now it has worked fine - if the set is empty then the top level assert looks for zero matches otherwise it checks there are the right number of matches and calls the block to validate them.

@joshuay03
Copy link
Contributor Author

@tomhughes I did not consider a mixed case like that...

The intention behind the change was to catch unintentional zero counts, for example:

expected_count = org.users.count # unintentionally 0 due to invalid setup
assert_select "user", count: expected_count do |users|
  # assertions that never run but seem like they're valid
  assert_equal "Josh", users.last.text
end

Although I'll admit I can see the usefulness in your case as well. I'll leave it to @rafaelfranca and @flavorjones to make the call on whether this change should be reverted.

If that doesn't happen, your helpers could be refactored to something like:

items = assert_select "item", :count => traces.length
if traces.length.positive?
  traces.zip(items).each do |trace, item|
    assert_select item, "title", trace.name
    ...
  end
end

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants