Skip to content

Raise an error when given a block with a 0 element assertion #116

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

Merged

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