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

Raise an error when calling custom matcher without using .and #1426

Open
AlecksJohannes opened this issue Aug 6, 2023 · 5 comments
Open

Comments

@AlecksJohannes
Copy link

AlecksJohannes commented Aug 6, 2023

Subject of the issue

Hi there folks 👋 ,

First of all, thank you for this great gem! I created this issue here just to offer a small idea, I encountered a small scenario when using custom matchers.

Let's say we have this custom matcher

RSpec::Matchers.define(:create_some_model) do
  chain(:method1) {}
  chain(:method2) {}
  chain(:method3) {}
  ...
end

describe do
  it do
    expect { some_action }.to create_some_model(Model).method1.method2.method3.create_some_model(Model2)
  end
end

As you can see, it is easy to make mistake as we sometime might forget to add .and between create_some_model and this could create false-positive test result as it will only run create_some_model(Model2). I understand that this is more human error but I would love to prevent it.

My idea is in RSpec::Matchers::DSL::Matcher, we also define

class RSpec::Matchers::DSL::Matcher
  define_method(method) do
    raise "You did not use .and between your 'create_some_model' test expectation"
  end
end

So calling create_some_model twice will resulting in error like that. There is one catch with this approach is you cannot call the same create_some_model inside the matcher itself but IMHO, I don't think it is a good idea to do such

RSpec::Matchers.define(:create_some_model) do
  chain(:method1) {}
  chain(:method2) {}
  chain(:method3) {}
  
 def something
   expect { }.to create_some_model <-- this doesn't work
 end
  ...
end

I can create a PR to apply this idea, please let me know what do you guys think. Thank you for reading!

P/S:

There is 1 more approach besides that is to get rid of the defined custom matcher method from RSpec::Matchers::DSL::Matcher but if we do that we also have to get rid of the method_missing as well which I don't think it is a good idea.

Your environment

  • Ruby version: 2.7.5
  • rspec-expectations version: 3.13.0.pre
@pirj
Copy link
Member

pirj commented Aug 6, 2023

Thanks for reporting! It’s certainly accidental behaviour.
Wondering if it’s possible to play with access levels to disallow explicit receiver.
I’m pretty sure there were cases where yhe matcher was called recursively. If it would be possible to keep this and issue a warning, that would be perfect.
A PR to fix this is welcome!

@AlecksJohannes
Copy link
Author

AlecksJohannes commented Aug 6, 2023

Thanks for reporting! It’s certainly accidental behaviour.
Wondering if it’s possible to play with access levels to disallow explicit receiver.
I’m pretty sure there were cases where yhe matcher was called recursively. If it would be possible to keep this and issue a warning, that would be perfect.
A PR to fix this is welcome!

Hello there! I hope you are enjoying your weekend ☺️

Surely, I had the same idea I will play around with access levels to see if I can somehow do that, otherwise if it is too complicated then I believe we can raise it as a warning instead of error (will also create a PR for that)

P/S (12/8/2023): I believe I have the solution, I will open a PR soon.

@AlecksJohannes
Copy link
Author

Hello there @pirj 👋 I hope you are doing well, I have opened a PR for this issue. Thank you!

@JonRowe
Copy link
Member

JonRowe commented Aug 16, 2023

The method should not even be available on the matcher for what its worth, e.g .eq(thing).eq will raise a NoMethodError

@AlecksJohannes
Copy link
Author

Hey folks, I hope you are all well!

I have left a comment here #1428 (comment), could you guys have a look ? Thank you

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

No branches or pull requests

3 participants