Rubocop-rspec: DescribeMethod and DSL methods

[mockdeep]

We're trying to figure out the best way to handle DSL methods that are being tested, such as rails associations. Right now we have spec/models/user/associations_spec.rb for grouping those, and a similar validations_spec.rb and whatnot. Any advice on how to structure these so that they can work with the DescribeMethod cop? Right now I'm thinking maybe we just make a spec/models/user/dsl folder and ignore that for the rule or something...

[nijikon]

@mockdeep Did you solve your issue? Can you share?

[mockdeep]

@nijikon Right now we just ended up just adding ignores for files matching particular patterns:

RSpec/DescribeMethod:
  Exclude:
    - spec/poros/constants/**/*
    - '**/after_save_spec.rb'
    - '**/after_validation_spec.rb'
    - '**/associations_spec.rb'
    - '**/before_destroy_spec.rb'
    - '**/before_save_spec.rb'
    - '**/callbacks_spec.rb'
    - '**/default_scope_spec.rb'
    - '**/delegations_spec.rb'
    - '**/validations_spec.rb'

We also have a constants.rb file where our describe blocks look like:

RSpec.describe Constants, '::DECIMAL_REGEX' do

so we just ignored that as well. I could see changing the rule to expect a dsl_methods folder where they are named as something like:

RSpec.describe SomeClass, 'dsl_method' do

# or maybe:

RSpec.describe SomeClass, '&dsl_method' do

As well as having a structure for namespaced constants as well. Could just be a constants folder with the above syntax.

[andyw8]

My gut feeling is that if you need multiple spec files for a single implementation file, then your class has too many responsibilities (an SRP violation). I don't think this is anything rspec-rubocop can help with.

[mockdeep]

Not sure if passing judgement on the way we write our tests adds anything to the conversation here. While we try to keep our classes pretty small, and often design classes with only a single call method, we still have other concerns like a constants file that have a number of constants we want to test against a variety of scenarios, or ActiveRecord classes with different scopes, validations, and relationships we want to test thoroughly. Having separate test files has been nice so that we don't have to go hunting through a single file to find the specs that are relevant to what we're working on at the moment.

Regardless, rspec-rubocop already supports the multiple file approach, so that's kind of irrelevant. The question was oriented towards dsl methods which aren't covered by the describe SomeClass, '#some_method' approach.

[andyw8]

@mockdeep The problem I see is that there's no existing community convention to follow. It sounds like your team has developed their own approach, but as it's not in wider use, it seems beyond the remit of rubocop-rspec to support that.

[mockdeep]

@andyw8 We didn't develop this approach ourselves, but found it in other open source projects and adopted it:

https://github.com/solnic/virtus/tree/master/spec/unit/virtus
https://github.com/dkubb/axiom/tree/master/spec/unit/range
https://github.com/mbj/unparser/tree/master/spec/unit/unparser/comments

Again, rubocop-rspec already supports this approach. The question isn't about the one-method-per-spec-file approach, but about how to handle naming of specs for DSL methods that we want to test.

[andyw8]

Thanks @mockdeep, I think I've got a better understanding of this now.

It seems the rules for the DescribeMethod cop should be as follows:

• If the second argument is not a string, do nothing.
• If the second begins with . or #, do nothing.
• Otherwise, ensure that the spec's file name matches the concatenated 'underscored' versions of the two arguments, e.g. Foo::Bar, 'cats and dogs' do should match foo/bar/cats_and_dogs_spec.rb.

Does that sound right?

[mockdeep]

In this case, we're not talking about arbitrary strings, but typically a constant set of structured names, such as delegations_spec, validations_spec, or associations_spec. Sometimes we'll want to a before_save_spec or the like. I don't think I'd actually want to permit arbitrary strings for the title in order to prevent inconsistencies, so I guess the best I can think of is there is a list of dsl_names that are acceptable either with some sort of preceding symbol, such as &, or just as-is. Then the names might be configurable via the .rubocop.yml file.

[andyw8]

There could be a configuration option, e.g.:

RSpec/DescribeMethod:
  IgnoreNames:
    - delegations
    - validations
    - associations

but I can't see any advantage of that approach over using Exclude:.

[mockdeep]

@andyw8 Yeah, I don't think ignoring them would be ideal. We're already ignoring the rule for certain filenames, but this is more about coming up with an enforced style for these kinds of things. I don't have a great idea for a generalizeable rule, aside from simply having a dsl directory and verifying that the spec name matches the file name.

[backus]

@mockdeep could you let me know if you are still looking for feedback / input? This issue is a bit old and I'm trying to reduce our open (stale) issue count. Would like to close this if you aren't requesting any changes / improvements

[mockdeep]

@backus I guess I was hoping for some sort of rubocop-rspec adjustments to be able to enforce conventions around these additional cases, but being that there isn't really any convention to refer to, I guess this can be closed. For now we're just ignoring this rule for those cases.

[backus]

@mockdeep alright. Personally I think that .class_method and #instance_method are clearly ruby conventions and are also useful for tools like mutant, so that is close enough IMO to a community convention. Feel free to ask questions about / challenge my assertion here I'm happy to discuss further. Closing this issue now though to reduce clutter

[mockdeep]

@backus I was referring more to the things that aren't class or instance methods, such as constants or testing dsl class method calls where you're not testing the method directly so much as that it gets called with a particular result. We're using '::SOME_REGEX' to describe constants and right now just 'has_many' for dsl methods, and ignoring those files from the rubocop-rspec rule.

[backus]

@mockdeep Hm so I don't know of many cases where I'm describing a constant. Why are you doing that? My temptation is to say that you should just encapsulate they behavior you want a constant to provide in a method that you are able to test. I'd be happy to be proven wrong though by an example.

For a class macro I still specify the class method syntax. For example a macro like test might look something like this: https://github.com/mbj/mutant/blob/master/spec/unit/mutant/ast/named_children_spec.rb#L2-L9

[mockdeep]

One example of a constant we test is like above where we'll have a regex of some sort where we write specs around it to make sure it covers the appropriate cases, such as Constants::EMAIL_REGEX. Wrapping it in a method to avoid this seems a little like the tail wagging the dog, since it's literally meant to be a constant, not changing and not really code logic, and referring to it as such in the code makes it more clear where it's coming from. Putting it in a method means you'd also want to memoize it to prevent it from being instantiated fresh with each method call, rather than initially when the class is loaded.

We did the class macro thing for a while but that didn't feel right, since it's conflating testing the method definition with testing usage of the method. We're not testing the implementation of the method itself. I kind of like the idea of referring to it as &macro_method to make a distinction between the two, maybe placing it under a class_macros directory...

[backus]

@mockdeep regarding constants

One example of a constant we test is like above where we'll have a regex of some sort where we write specs around it to make sure it covers the appropriate cases, such as Constants::EMAIL_REGEX. Wrapping it in a method to avoid this seems a little like the tail wagging the dog, since it's literally meant to be a constant, not changing and not really code logic, and referring to it as such in the code makes it more clear where it's coming from. Putting it in a method means you'd also want to memoize it to prevent it from being instantiated fresh with each method call, rather than initially when the class is loaded.

Ah so I didn't mean to just create a method which returns a regex. My point is that you probably don't need to expose a regular expression to the world. In other words, does every user of EMAIL_REGEX need an object that responds to #==, #===, #=~, #casefold?, #encoding, #eql?, #fixed_encoding?, #hash, #inspect, #match, #named_captures, #names, #options, #source, #to_s, and #~? Or do they just need to know whether a string is an email? Therefore, I'm saying instead do something like this:

class MyApp
  EMAIL_REGEX = /\A[\w+\-.]+@[a-z\d\-]+(\.[a-z]+)*\.[a-z]+\z/i
  private_constant(:EMAIL_REGEX)

  def self.email?(input)
    EMAIL_REGEX === input
  end
end

then you should test that email? method. Narrow interface, testable, more refactor friendly, etc.

[backus]

@mockdeep regarding class macros

it's conflating testing the method definition with testing usage of the method.

You'll have to clarify what you mean. Could you pluck a real life example out of your codebase perhaps?

[mockdeep]

@backus any of the ActiveRecord macro methods are examples of testing the use of the method as opposed to testing the implementation of the method. For example when I'm testing validates, I'm not testing that validates does what I think it should, which I'm trusting is tested somewhere in the ActiveRecord codebase, but at a higher level that I'm actually validating. Here's the distinction in a simple code example:

class A
  def self.hold_this_for_me(thing)
    @thing = thing
  end

  def self.holding?(thing)
    @thing == thing
  end
end

class B < A
  hold_this_for_me 'pizza'
end

RSpec.describe A, '.hold_this_for_me' do
  it 'holds something' do
    A.hold_this_for_me('shoe')
    expect(A).to be_holding('shoe')
    expect(A).not_to be_holding('badger')
    A.hold_this_for_me('badger')
    expect(A).to be_holding('badger')
    expect(A).not_to be_holding('shoe')
  end
end

RSpec.describe B, '&hold_this_for_me' do
  it 'holds a pizza' do
    expect(B).to be_holding('pizza')
  end
end

[backus]

Hm so we might just differ in opinion here. With that said, here are two reasons I'm not a fan of the &hold_this_for_me syntax:

1. The ampersand has syntactic meaning in ruby. In many cases you can interpret &foo as &foo.to_proc:

``` ruby
module Square
def self.to_proc
method(:call).to_proc
end

 def self.call(number)
   number ** 2
 end

end

[2, 3, 4, 5].map(&Square) # => [4, 9, 16, 25]
```

1. In terms of coverage your example really has nothing to do with .hold_this_for_me. I realize you are testing state and your convention is that you are saying that .hold_this_for_me introduced this state and therefore &hold_this_for_me signifies that you are asserting behavior about state produced by that method.

So I think it is neat that your team has a convention for this. With that said, while I get what you guys are going for, but I don't think it would ever warrant making DescribeMethod customizable because I think it would confuse more users than it would benefit.

(Looking forward to your ::CONST convention response)

[mockdeep]

Re: 1. & was just one idea which kind of suggested to me "the calling of...", but I'm not too particular on the signifier so much as just having a convention. Though, the same could be said of . as a signifier, since it has quite a bit of meaning as well.

Re: 2. I don't think it has _nothing_ to do with .hold_this_for_me, but it is a somewhat less clear case of how you should signify what you're testing. With validations I have at times used #valid? or #validate, but that's not quite there, either, since I never actually call #valid? on the record and I'm kind of just assuming ActiveRecord calls something like that under the hood. Not to mention I think it's nice to have a distinction between methods actually defined on the record and things that are configured via macro methods.

Sorry, I missed your comment about constants. In some cases it might make sense to encapsulate that logic in a method somewhere, but we often still want to use a particular constant in multiple locations and in multiple ways. For example, with ::EMAIL_REGEX we might want to grab emails out of an arbitrary string or check whether something is an email or not. Or we have Constants::PER_PAGE for the number of records we show on a page that gets used in a lot of places. We could find ways to wrap these in some sort of object or global function, but it's nice having the convenience of being able to access these things where we need them, rather than having to build some wrapper to handle it for us. Even moreso when those constants are literally that and nothing more, such as a list of internal emails, ::EMAILS.fetch(:support), ::EMAILS.fetch(:sales).

[backus]

@mockdeep Thanks for all of the detail here. For your constant convention I might recommend just doing something like

RSpec/DescribeMethod:
  Exclude:
    - 'spec/my_app/constants/**/*_spec.rb'

I understand your use case now but I still think that tests should be applied to methods instead of constants since that is more in the spirit of "testing behavior" vs. "testing implementation."

[backus]

@mockdeep Also for your ActiveRecord validations why don't you just instantiate the objects, call #vaild?, and then check errors[:attribute_in_questino] in your tests? If you did that then I would just not put anything for the second argument of describe. You are testing the overall semantics of your object not one particular method

[mockdeep]

@backus Yeah, we're just ignoring specific files where the rule doesn't apply for now. It's not so ideal, though, because then we don't have a means of enforcing a convention on those as well.

Regarding the #valid? bit, having a second argument allows us to group things based on what they represent in our tests. We have validations, relations, delegations, and so on, so just lumping them under the top level group would reduce the clarity, there. We're using shoulda-matchers to test the less complicated ones, and for more complicated ones we do something like what you mention.

[backus]

Interesting. Well you can always write and include a custom cop if you want to enforce a domain specific convention. As I said before configuring DescribeMethod doesn't seem like the solution to me. You can very likely base your own cop on it though and just disable DescribeMethod and make MyDomain/DescribeMethod which would just be the current source but with a modified regexp

[mockdeep]

Yeah, I might get around to building a custom rule for it at some point.