`have_public_method` and `have_protected_method` matchers are missing

[trinistr]

There are matchers for pretty much all other variations of method-having, but there are none for #public_methods and #protected_methods:

$ ls lib/mspec/matchers/have*method*
lib/mspec/matchers/have_instance_method.rb
lib/mspec/matchers/have_method.rb
lib/mspec/matchers/have_private_instance_method.rb
lib/mspec/matchers/have_private_method.rb
lib/mspec/matchers/have_protected_instance_method.rb
lib/mspec/matchers/have_public_instance_method.rb
lib/mspec/matchers/have_singleton_method.rb

I would like to add these matchers for completeness.

Rationale:

• I think anyone can reasonably expect to have these matchers available.
• There are tests that would benefit (see for example core/argf/shared/each_codepoint.rb).

[trinistr]

I would like to add these matchers for completeness.

A PR is already in the works, mostly just wanted to get opinion if this is something that shouldn't be added.

[eregon]

• There are tests that would benefit (see for example core/argf/shared/each_codepoint.rb).

Like https://github.com/ruby/spec/blob/b833642f457a047aea80bb8157e343d10c80c3ea/core/argf/shared/each_codepoint.rb#L13

I think that should be this rather:

@argf.public_methods(false).should.include?(@method)

In general I don't like those have*method matchers because they are rather unclear what they actually do/check, and just using e.g. public_methods(false) seems far clearer.
https://github.com/ruby/mspec/blob/bd8efcf2951b99c9c2235d1ddb7ddb06f37aa332/lib/mspec/matchers/have_private_method.rb for example just seems unnecessary boilerplate, it just calls .private_methods, so why a matcher jut for that? It's also error-prone as it calls private_methods with default true which is typically not what we want in specs.

For these reasons I think we should actually deprecate all these have*method matchers, but unsure it's worth the effort.

One step would be to update https://github.com/ruby/spec/blob/master/CONTRIBUTING.md#predicate-matchers to no longer mention them.

Sorry, I guess that's not what you were looking for but yeah in the current state I see these matchers as legacy stuff that's just waiting to be deprecated. Probably the same for all have_ matchers, all of these seem trivially expressive without a custom matcher, and the error message would still be good enough when it fails.

[eregon]

We should probably also deprecate be_kind_of and be_an_instance_of, they are redundant with .should.foo form:

obj.should be_kind_of(Enumerable)
obj.should.is_a?(Enumerable)

obj.should be_an_instance_of(Array)
obj.should.instance_of?(Array)

[trinistr]

In my opinion, matchers that improve readability should be left in (and expanded). (But I'm also a fan of RSpec.)

have*method matchers, while making reading tests easier, indeed have somewhat of a problem with "what does this do?", though I would argue that "it just calls object.*methods(include_super).include?" is quite clear, if it is documented.

just seems unnecessary boilerplate

Eh, if the base class was rewritten, I'm pretty sure all the matchers could be one-liners, basically just HavePrivateMethodMatcher = MethodMatcher.build(:private_methods, "private method"), or even one call with a hash to create all of them. (Separation of concerns is questionable, but 'tis a reasonable trade-off.)

But I agree that these matchers may be superfluous, and the benefit is not that large to have them.

It's also error-prone as it calls private_methods with default true which is typically not what we want in specs.

Today I learned ™️ that SomeClass.private_methods(false) doesn't actually return only singleton methods of that class, but also of singleton superclasses (https://bugs.ruby-lang.org/issues/8743), and so usually also isn't what we actually want to test. Maybe there should instead be matchers have_own*class_method or something? It's currently not obvious that private_methods(false) is not the right tool (because documentation is wrong) and how to test this correctly.

(Going off on a tangent, how did it happen that private_class_method and private_instance_method do completely different things? Method-related methods remind me of PHP, and that is sad.)

We should probably also deprecate be_kind_of and be_an_instance_of, they are redundant with .should.foo form

I feel that obj.should be_kind_of(Enumerable) reads much easier than obj.should.is_a?(Enumerable), which is plain awkward to look at. should.instance_of? is more or less in the same boat.

On that note, are be_true and be_false deprecated? They seem completely unnecessary, though I still use them due to muscle memory from RSpec.

[eregon]

I feel that obj.should be_kind_of(Enumerable) reads much easier than obj.should.is_a?(Enumerable), which is plain awkward to look at. should.instance_of? is more or less in the same boat.

It's subjective for sure.
I explained my view of this in https://eregon.me/blog/2019/10/07/a-new-should-syntax.html

Maybe a clearer example is array.should include(42), that seems pretty bad because it relies on some include method defined "somewhere" which might conflict with e.g. Module#include.

The most important point I think is it must be clear which method is used as much as possible, so array.should.include?(42), a.should == b, mod.public_methods(false).should.include? :foo are IMO clearly better.

Many MSpec matchers are more or less a legacy thing at this point.
Some matchers still make sense when it's not trivially expressible with a single .should.some_method, but that isn't the case AFAIK for have_*_method.

Regarding https://bugs.ruby-lang.org/issues/8743 it would be helpful to have a concrete example. If it's not easily achievable without a matcher, a matcher may make sense. OTOH it would be very easy to forget using that matcher so the value of the matcher seems not high.

On that note, are be_true and be_false deprecated? They seem completely unnecessary, though I still use them due to muscle memory from RSpec.

I think it would make sense to deprecate and migrate usages of all matchers which can easily be replaced by .should.foo.
I haven't done it so far because it's quite a big change, and I wanted to wait some time after https://eregon.me/blog/2019/10/07/a-new-should-syntax.html (that's already 6 years ago now) to see if there is any drawback of the new syntax (I didn't find any).

[eregon]

FYI I filed ruby/spec#1350 to migrate to .should.foo for most matchers.

[trinistr]

Regarding https://bugs.ruby-lang.org/issues/8743 it would be helpful to have a concrete example. If it's not easily achievable without a matcher, a matcher may make sense. OTOH it would be very easy to forget using that matcher so the value of the matcher seems not high.

Basically, any tests of singleton methods on classes/modules suffer from the problem of "module has more methods than expected", especially when testing subclasses. Just a quick example:

class A
  def self.m = nil
end
A.public_methods(false)
=> [:m, :yaml_tag, :allocate, :superclass, :subclasses, :attached_object, :new]

class B < A
  def self.n = nil
end
B.public_methods(false)
# All these methods are still here!
=> [:n, :m, :yaml_tag, :allocate, :superclass, :subclasses, :attached_object, :new]

So, there are two possible problems:

1. False-positive when testing for a presence of a method with a typo. Unlikely, but can happen with fixture classes.
2. Unexpected positive when testing that a method is not present. I'm forgetting what I was testing exactly, but I was bewildered when it happened due to this bug(?).

I will close this issue, since the original proposal is no longer relevant, but please let me know your thoughts on this pesky issue of class methods.

[eregon]

That example is helpful, thanks.
I think it's unlikely enough to have an issue with this, but for the rare cases it matters one can use:

(B.public_methods(false) - Class.instance_methods(false)).should/should_not.include? :foo