Allow `satisfy` to match the block expectation return value #1477

pirj · 2024-08-04T17:06:00Z

ary = [1, 2]
expect { ary.shift }
  .to change { ary }.to([2])
  .and satisfy { |returned_value| returned_value == 1 }

pirj · 2024-08-18T11:49:11Z

cc @zverok WDYT?

JonRowe

Sorry for the delay reviewing this @pirj definietly a good idea and good work but needs some tweaks

JonRowe · 2024-08-06T11:56:44Z

features/built_in_matchers/satisfy.feature

@@ -39,3 +53,20 @@ Feature: `satisfy` matcher
      | expected 10 to satisfy expression `v > 15`    |
      | expected 10 not to be greater than 5          |
      | expected 10 to be greater than 15             |
+
+  @skip-when-ripper-unsupported


Did you just copy this tag or check to see if it works without it? I'd expect it to work without it as its not matching source.

JonRowe · 2024-08-06T11:57:53Z

lib/rspec/matchers.rb

+    # Passes if the submitted block returns true.
+    # For value expectations, yields target to the block.
+    # For block expectations, yields the expectation's returned value to the block.


Suggested change

# Passes if the submitted block returns true.

# For value expectations, yields target to the block.

# For block expectations, yields the expectation's returned value to the block.

# Passes if the submitted block returns true. Yields targer to the

# block. This is either the direct value or the result of an `expect` block.

JonRowe · 2024-08-06T11:58:59Z

Changelog.md

@@ -11,6 +11,7 @@ Enhancements:

 * Improve the IO emulation in the output capture matchers (`output(...).to_stdout` et al)
  by adding `as_tty` and `as_not_tty` to change the `tty?` flags. (Sergio Gil Pérez de la Manga, #1459)
+* Allow `satisfy` to match block expectations return value. (Phil Pirozhkov, #1477)


Suggested change

* Allow `satisfy` to match block expectations return value. (Phil Pirozhkov, #1477)

* Improve the `satisfy` matcher to allow assertations on an `expect { ... }` return value.

(Phil Pirozhkov, #1477)

JonRowe · 2024-08-20T21:17:50Z

spec/support/shared_examples/value_matcher.rb

@@ -1,3 +1,19 @@
+RSpec.shared_examples "an RSpec value-only matcher" do |options|


I know this technically makes it more consistent but it causes a lot of churn, I'm leaning towards preferring inverting this so that "an RSpec value matcher" is the "value only" specs and the opposite is named something else, or even make these optional specs part of one group like above.

JonRowe · 2024-09-07T09:42:02Z

spec/rspec/matchers/built_in/satisfy_spec.rb

+    end
+  end
+
+  pending "allows a matcher as an argument" do


I'd rather not introduce pending specs unless its for something version specific, what needs to be done to make this work?

JonRowe · 2024-09-07T09:43:04Z

spec/rspec/matchers/built_in/satisfy_spec.rb

+    end
+  end
+
+  describe "composed usage" do


This should follow on from the example group description

Suggested change

describe "composed usage" do

describe "when composed with other matchers" do

JonRowe · 2024-09-07T09:44:56Z

lib/rspec/matchers/built_in/compound.rb

-              @match_results[inner] = inner.matches?(inner_matcher_block(args))
+              p = Proc.new { |*args2|
+                returned = inner_matcher_block(args).call(*args2)
+              }


This needs to take care of ruby2 keywords (e.g. marked as such, not having keywords itself) so that they get passed through, I'm also a bit nervous about returned why isn't it just implict from the assignment?

zverok · 2024-09-08T15:03:12Z

(A bit of a philosophical note, as I was tagged...)

I can’t say that I am fond of this solution. The original idea for return_value matcher, for me, is more in the spirit of RSpec (and I actually have one in saharspec, called just ret, as you are probably aware).

The solution with satisfy { boolean check } seems to go against the RSpec spirit of “every check should be built from combining matchers,” the spirit that allows to produce clear explanations of “what is expected” in --format doc mode, but which requires to write matchers in “RSpec DSL” instead of pure Ruby (e.g., we write expected.to eq 5 not expected.to == 5).

This is a choice that is questioned by some, but it allows to build powerful, combinable checks, which very clearly report “what went wrong” when the specs are red, able to produce nice diffs depending on the context of the check and so on.

Even the original satisfy { any code } looked like an “escape hatch” for me (basically, “when you are in a hurry, or the check is so unique that it is easier then make a new small clear matcher”), and extending its usage for new cases... I am not sure if I see it justified.

May I ask why the original return_value(other_matcher) idea was abandoned?..

pirj force-pushed the return_value-matcher branch from 8560395 to 690f7cd Compare August 4, 2024 17:08

pirj changed the title ~~Teach satisfy to match on the block expectation return value~~ Allow satisfy to match the block expectation return value Aug 4, 2024

pirj marked this pull request as ready for review August 4, 2024 17:10

pirj self-assigned this Aug 4, 2024

pirj requested a review from JonRowe August 4, 2024 17:11

Allow satisfy to match block expectations return value

bd5017c

pirj force-pushed the return_value-matcher branch from 778e977 to bd5017c Compare August 4, 2024 22:00

JonRowe requested changes Sep 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow `satisfy` to match the block expectation return value #1477

Allow `satisfy` to match the block expectation return value #1477

pirj commented Aug 4, 2024 •

edited

Loading

pirj commented Aug 18, 2024

JonRowe left a comment

JonRowe Aug 6, 2024

JonRowe Aug 6, 2024

JonRowe Aug 6, 2024

JonRowe Aug 20, 2024

JonRowe Sep 7, 2024

JonRowe Sep 7, 2024

JonRowe Sep 7, 2024

zverok commented Sep 8, 2024

	* Allow `satisfy` to match block expectations return value. (Phil Pirozhkov, #1477)
	* Improve the `satisfy` matcher to allow assertations on an `expect { ... }` return value.
	(Phil Pirozhkov, #1477)

		@@ -1,3 +1,19 @@
		RSpec.shared_examples "an RSpec value-only matcher" do \|options\|

	describe "composed usage" do
	describe "when composed with other matchers" do

Allow satisfy to match the block expectation return value #1477

Are you sure you want to change the base?

Allow satisfy to match the block expectation return value #1477

Conversation

pirj commented Aug 4, 2024 • edited Loading

pirj commented Aug 18, 2024

JonRowe left a comment

Choose a reason for hiding this comment

JonRowe Aug 6, 2024

Choose a reason for hiding this comment

JonRowe Aug 6, 2024

Choose a reason for hiding this comment

JonRowe Aug 6, 2024

Choose a reason for hiding this comment

JonRowe Aug 20, 2024

Choose a reason for hiding this comment

JonRowe Sep 7, 2024

Choose a reason for hiding this comment

JonRowe Sep 7, 2024

Choose a reason for hiding this comment

JonRowe Sep 7, 2024

Choose a reason for hiding this comment

zverok commented Sep 8, 2024

Allow `satisfy` to match the block expectation return value #1477

Allow `satisfy` to match the block expectation return value #1477

pirj commented Aug 4, 2024 •

edited

Loading