Skip to content
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

Forward given filter block to matcher #2617

Conversation

cbliard
Copy link
Contributor

@cbliard cbliard commented Nov 18, 2022

Fixes #2616

@twalpole
Copy link
Member

Sorry for the delay, this will be looked at/considered this coming weekend

@twalpole
Copy link
Member

I don't think the tests added are actually validating the filter blocks are run since the have_selector calls would pass with or without the filter blocks.

@cbliard
Copy link
Contributor Author

cbliard commented Dec 27, 2022

You're right. When writing the tests, I knew that some of them were not actually accurate as they would pass with or without the block.

This one for instance will happily pass with or without the block:

expect(page).to have_selector(:css, 'input#test_field') { |input| input.value == 'monkey' }

Contrary to this one which would not pass without the block:

expect(page).to have_no_selector(:css, 'input#test_field') { |input| input.value == 'not a monkey' }

How should the tests be formulated then? I saw in lib/capybara/spec/session/element/match_css_spec.rb that some tests are written like expect { expect(page).to match_something }.to raise_exception(RSpec::Expectations::ExpectationNotMetError). Following it, the test can be written like this:

expect { expect(page).to have_selector(:css, 'input#test_field') { |input| input.value == 'not a monkey' } }
  .to raise_exception(RSpec::Expectations::ExpectationNotMetError)

Would it be better? Any other suggestions?

@twalpole
Copy link
Member

Either that or changing the tests to consider number of matched elements like

expect(page).to have_selector(:css, 'input', count: 1) { |input| input.value == 'monkey' }

which would be easier to read/understand

@twalpole
Copy link
Member

twalpole commented Dec 31, 2022

Looking at this further, this breaks the default binding of the do block to to which is defined to allow a proc for failure messages. Not really sure what the best solution here actually is. This PR is overriding Rubys default binding precedence and could break existing code... Ignore, I mistread the RSpec code

@twalpole
Copy link
Member

Merged via cacd032

@twalpole twalpole closed this Dec 31, 2022
@cbliard
Copy link
Contributor Author

cbliard commented Jan 3, 2023

Thanks @twalpole for merging.

I'm still feeling bad about the tests. Do you mind if I open another PR to provide better tests with count: 1 as you suggested?

@twalpole
Copy link
Member

twalpole commented Jan 7, 2023

@cbliard I had already fixed the tests

sandbergja added a commit to projectblacklight/blacklight that referenced this pull request Apr 19, 2023
* Use updated capybara filter_block syntax

teamcapybara/capybara#2617 changed
how blocks are evaluated.  Thanks to @maxkadel for finding
this change!

* rubocop -a

* ViewComponent 3 compatibility: don't call i18n methods in a component initializer

This is due to this upstream PR: ViewComponent/view_component#1666
sandbergja added a commit to sandbergja/blacklight that referenced this pull request Apr 27, 2023
* Use updated capybara filter_block syntax

teamcapybara/capybara#2617 changed
how blocks are evaluated.  Thanks to @maxkadel for finding
this change!

* rubocop -a

* ViewComponent 3 compatibility: don't call i18n methods in a component initializer

This is due to this upstream PR: ViewComponent/view_component#1666
cbeer pushed a commit to projectblacklight/blacklight that referenced this pull request Aug 10, 2023
* Use updated capybara filter_block syntax

teamcapybara/capybara#2617 changed
how blocks are evaluated.  Thanks to @maxkadel for finding
this change!

* rubocop -a

* ViewComponent 3 compatibility: don't call i18n methods in a component initializer

This is due to this upstream PR: ViewComponent/view_component#1666
cbeer pushed a commit to projectblacklight/blacklight that referenced this pull request Aug 10, 2023
* Use updated capybara filter_block syntax

teamcapybara/capybara#2617 changed
how blocks are evaluated.  Thanks to @maxkadel for finding
this change!

* rubocop -a

* ViewComponent 3 compatibility: don't call i18n methods in a component initializer

This is due to this upstream PR: ViewComponent/view_component#1666
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Different behavior between {..} and do..end block with filter block of rspec matchers
2 participants