Allow count constraint on `include` matcher #1168

marcandre · 2020-03-17T04:14:53Z

This allows to specify how many times an element should be included:

expect([1, 2, 1]).to include(1).at_most(3).times
expect('hello').to include(/l/).twice

Note: Not implemented for include(1, 2, 3) nor for hash cases.
Also: This PR build on #1167.

pirj

Awesome!

features/built_in_matchers/include.feature

pirj · 2020-03-18T06:51:56Z

lib/rspec/matchers/built_in/count_expectation.rb

+      # @api private
+      # Asbtract class to implement `once`, `at_least` and other
+      # count constraints.
+      class CountExpectation < BaseMatcher


👏 I took a stab at this in the past, but your implementation is superior by all means.
My aim was to reuse it in rspec-mocks's recieve, do you think it's doable in theory by making it an includable module? rspec-mocks don't depend on rspec-expectations, and rspec-support doesn't seem to be a good place for CountExpectation.

I looked at receive first too and got discouraged as the implementation is more complex, needs to care about block passing, deals with other constraints, etc. and it wasn't in the same gem. When I realized I could use yield's code, that was simple.

I agree, all of that code could be using the same implementation, although in practice it's not clear if there would be that much gain.

I first used a module BTW, but yard seemed to have issues with the module having an initialize method so instead went with a class.

I've seen this code being repeated in custom matchers quite a lot. E.g. https://github.com/zverok/saharspec/blob/master/lib/saharspec/matchers/send_message.rb#L31

Should I turn it into a module? Probably should avoid initialize altogether if it's to be made publicly and rely on attr_readers I imagine

Maybe add attr_reader for count_expectation_type/count_expected_count? They would default to nil and that would make it possible to get rid of initialize.

Just for the reference WIP.

lib/rspec/matchers/built_in/include.rb

lib/rspec/matchers/built_in/count_expectation.rb

pirj · 2020-03-18T07:58:19Z

Looks good to me. CounterExpectation is private, so we don't have any obligation to keep it as a class, so it can be turned into an includable module in the futute.

JonRowe · 2020-03-19T14:10:07Z

👋 I've had a brief look over this, but delayed reviewing in depth because I knew I wanted to support combination constraints on the yield matcher that would have knock on effects.

Could you rebase / rejig this so it works with support that combination? If you have the time I'd love to see the logic from yield pulled out into something thats used by the other matchers with composition rather than inheritance...

marcandre · 2020-03-20T04:22:11Z

👋 I've had a brief look over this, but delayed reviewing in depth because I knew I wanted to support combination constraints on the yield matcher that would have knock on effects.

Could you rebase / rejig this so it works with support that combination? If you have the time I'd love to see the logic from yield pulled out into something thats used by the other matchers with composition rather than inheritance...

Sounds good. I'll be busy for the next few days but will ping when I push the updated PR

pirj · 2020-03-26T14:49:09Z

Another matcher that might benefit from the extraction https://github.com/rspec/rspec-rails/blob/master/lib/rspec/rails/matchers/active_job.rb#L167

marcandre · 2020-03-28T05:54:01Z

Refactored a bit & rebased.

pirj

Awesome! ⚡️

lib/rspec/matchers/built_in/count_expectation.rb

lib/rspec/matchers/built_in/include.rb

pirj · 2020-05-11T20:28:44Z

@JonRowe mind taking another look?

benoittgt

LGTM. I find the code, clean and self documented. Good job @marcandre. 🙌🏻

pirj · 2020-06-11T08:01:36Z

@JonRowe ?

marcandre · 2020-06-26T23:45:11Z

Ping @JonRowe

Add the aria-describedby id to the select_all_that_apply field as it broke WAVE testing of aria tags Turn the select_all_that_apply field to a hint text to standardise with other pages such as the identify_types_of_outgoings page Fix duplication of error messages under the select_all_that_apply hint because it's already in the error summary above the title. This is only when checkboxes are checked and nested values cause an error on submission. It still shows an error underneath it for when no checkboxes were selected validation fails Test the above. However, keep an eye on rspec/rspec-expectations#1168 which is an rspec matcher coming soon that allows to test how many times something has been seen in a response body.

Add the aria-describedby id to the select_all_that_apply field as it broke WAVE testing of aria tags for the savings and investments page and other assets page. Fix duplication of error messages under the select_all_that_apply hint because it's already in the error summary above the title. This is only when checkboxes are checked and nested values cause an error on submission. It still shows an error underneath it for when no checkboxes were selected validation fails Test the above. However, keep an eye on rspec/rspec-expectations#1168 which is an rspec matcher coming soon that allows to test how many times something has been seen in a response body.

marcandre · 2020-08-26T19:43:50Z

Rebased, just to be sure.

@JonRowe could this be merged please?

JonRowe

One outstanding query, but it's not a blocker.

lib/rspec/matchers/built_in/include.rb

pirj · 2020-08-29T13:34:27Z

Thank you! 👏

I understand that you're scratching your own itch to allow include to accept counts, but from my point of the most valuable part is the extraction of CountExpectation that can be used in a lot more matchers, and become a part of the public API for the benefit of custom matchers. I've seen so many custom matchers using their own incomplete implementations of CountExpectation.

marcandre force-pushed the include_count_constraint branch from 47d17de to 993e363 Compare March 17, 2020 04:15

marcandre mentioned this pull request Mar 17, 2020

include matcher does not handle duplicates (or overlapping matchers) well #685

Open

marcandre force-pushed the include_count_constraint branch from 993e363 to 7d7374d Compare March 17, 2020 04:55

marcandre mentioned this pull request Mar 17, 2020

yield_control hardening #1167

Merged

marcandre force-pushed the include_count_constraint branch 6 times, most recently from ebd6738 to e7fd3ab Compare March 17, 2020 20:55

pirj approved these changes Mar 18, 2020

View reviewed changes

marcandre force-pushed the include_count_constraint branch 2 times, most recently from ea43fa1 to c9efca0 Compare March 18, 2020 07:42

pirj reviewed Mar 18, 2020

View reviewed changes

lib/rspec/matchers/built_in/count_expectation.rb Outdated Show resolved Hide resolved

marcandre force-pushed the include_count_constraint branch 2 times, most recently from ab65187 to ac62d6f Compare March 18, 2020 07:49

marcandre force-pushed the include_count_constraint branch from ac62d6f to 42e80b3 Compare March 18, 2020 08:10

pirj requested a review from JonRowe March 18, 2020 09:16

marcandre force-pushed the include_count_constraint branch 5 times, most recently from 11584b1 to 2e314ee Compare March 28, 2020 05:41

pirj approved these changes Mar 29, 2020

View reviewed changes

marcandre force-pushed the include_count_constraint branch from 2e314ee to 05a0622 Compare March 29, 2020 20:18

pirj requested a review from benoittgt May 11, 2020 20:27

benoittgt approved these changes May 11, 2020

View reviewed changes

JonRowe changed the base branch from master to main August 2, 2020 02:06

Obsiye mentioned this pull request Aug 12, 2020

AP1540 Accessibility fix for Savings and Investments and other assets page ministryofjustice/laa-apply-for-legal-aid#1662

Merged

6 tasks

marcandre added 2 commits August 26, 2020 13:50

Make count expectation of yield_control generic

6c8e7c3

Implement count constraints for include matcher

d708270

marcandre force-pushed the include_count_constraint branch from 05a0622 to d708270 Compare August 26, 2020 17:50

JonRowe approved these changes Aug 28, 2020

View reviewed changes

lib/rspec/matchers/built_in/include.rb Show resolved Hide resolved

JonRowe merged commit be9b97c into rspec:main Aug 28, 2020

JonRowe mentioned this pull request Aug 30, 2020

Add benchmark for count vs select size #1212

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow count constraint on `include` matcher #1168

Allow count constraint on `include` matcher #1168

marcandre commented Mar 17, 2020

pirj left a comment

pirj Mar 18, 2020

marcandre Mar 18, 2020

pirj Mar 18, 2020

marcandre Mar 18, 2020

pirj Mar 18, 2020

pirj Mar 18, 2020

pirj commented Mar 18, 2020

JonRowe commented Mar 19, 2020

marcandre commented Mar 20, 2020

pirj commented Mar 26, 2020

marcandre commented Mar 28, 2020

pirj left a comment

pirj commented May 11, 2020

benoittgt left a comment

pirj commented Jun 11, 2020

marcandre commented Jun 26, 2020

marcandre commented Aug 26, 2020

JonRowe left a comment

pirj commented Aug 29, 2020

Allow count constraint on include matcher #1168

Allow count constraint on include matcher #1168

Conversation

marcandre commented Mar 17, 2020

pirj left a comment

Choose a reason for hiding this comment

pirj Mar 18, 2020

Choose a reason for hiding this comment

marcandre Mar 18, 2020

Choose a reason for hiding this comment

pirj Mar 18, 2020

Choose a reason for hiding this comment

marcandre Mar 18, 2020

Choose a reason for hiding this comment

pirj Mar 18, 2020

Choose a reason for hiding this comment

pirj Mar 18, 2020

Choose a reason for hiding this comment

pirj commented Mar 18, 2020

JonRowe commented Mar 19, 2020

marcandre commented Mar 20, 2020

pirj commented Mar 26, 2020

marcandre commented Mar 28, 2020

pirj left a comment

Choose a reason for hiding this comment

pirj commented May 11, 2020

benoittgt left a comment

Choose a reason for hiding this comment

pirj commented Jun 11, 2020

marcandre commented Jun 26, 2020

marcandre commented Aug 26, 2020

JonRowe left a comment

Choose a reason for hiding this comment

pirj commented Aug 29, 2020

Allow count constraint on `include` matcher #1168

Allow count constraint on `include` matcher #1168