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

Add a cop that detects constant, class, and module declarations in DSL #765

Merged
merged 2 commits into from May 21, 2019
Merged

Add a cop that detects constant, class, and module declarations in DSL #765

merged 2 commits into from May 21, 2019

Conversation

@pirj
Copy link
Member

@pirj pirj commented May 10, 2019

Those declarations are defined in the global namespace and leak between examples.

Fixes #197

Related to #750

Credit goes to:

  • @jonatas, the original creator.
  • several engineers working for Toptal at the time for noticing and pinning down the original problem

I've changed the original cop created for in-house use to comply with the strict coding style of this repository.


Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the changelog if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

  • Added the new cop to config/default.yml.
  • The cop documents examples of good and bad code.
  • The tests assert both that bad code is reported and that good code is not reported.
@pirj
Copy link
Member Author

@pirj pirj commented May 10, 2019

@dgollahon Appreciate if you take a look. I'm reading your comments to #381 ATM.

Loading

@pirj
Copy link
Member Author

@pirj pirj commented May 10, 2019

before do
  stub_const('Foo', Class.new)
  class Foo
    # define the class now
  end
end

that you mention can be written differently, by defining the method inside a block passed to Class.new, e.g.

before do
  fake_class = Class.new do
    # define the class now
  end
  stub_const('Foo', fake_class)
end

Do you think that it's a frequent situation when some methods are defined in a block, and at the same time examples/contexts need to add/redefine methods on that class?

cc @backus

Loading

@pirj
Copy link
Member Author

@pirj pirj commented May 17, 2019

@bquorning please take a look.

Loading

Copy link
Collaborator

@bquorning bquorning left a comment

What to do when I want to instantiate the class inside an example? That is not possible with stub_const, right? (e.g. RuboCop::RSpec::FakeCop)

Loading

lib/rubocop/cop/rspec/leaky_constant_declaration.rb Outdated Show resolved Hide resolved
Loading
spec/rubocop/cop/rspec/leaky_constant_declaration_spec.rb Outdated Show resolved Hide resolved
Loading
Loading
spec/rubocop/cop/rspec/leaky_constant_declaration_spec.rb Outdated Show resolved Hide resolved
Loading
@pirj
Copy link
Member Author

@pirj pirj commented May 18, 2019

What to do when I want to instantiate the class inside an example? That is not possible with stub_const, right? (e.g. RuboCop::RSpec::FakeCop)

You can, e.g.:

  before do
    fake_class = Class.new(described_class) do
                   def do_something
                   end
                 end
    stub_const('OtherClass', fake_class)
  end
  let(:instance) { fake_class.new }

The reason this is not possible with FakeCop is different, I've described the reason in the message for "Disable LeakyConstantDeclaration" commit.

Loading

pirj added 2 commits May 20, 2019
It seems to be impossible to define a cop with an anonymous class, since
`inherited` hook on `RuboCop::Cop::RSpec::Cop` is called before class
body is evaluated, and it's impossible to define class name.

    before do
      fake_cop = Class.new(described_class) do # <- fails here
                   # This never gets called
                   def self.name
                     'RuboCop::RSpec::FakeCop'
                   end

                   def on_send(node)
                     add_offense(node, location: :expression, message: 'I flag everything')
                   end
                 end

      stub_const('RuboCop::RSpec::FakeCop', fake_cop)
    end

    NoMethodError:
      undefined method `split' for nil:NilClass
    # rubocop-rspec/gems/rubocop-0.65.0/lib/rubocop/cop/badge.rb:26:in `for'
    # rubocop-rspec/gems/rubocop-0.65.0/lib/rubocop/cop/cop.rb:60:in `badge'
    # rubocop-rspec/gems/rubocop-0.65.0/lib/rubocop/cop/registry.rb:34:in `enlist'
    # rubocop-rspec/gems/rubocop-0.65.0/lib/rubocop/cop/cop.rb:56:in `inherited'
    # ./lib/rubocop/cop/rspec/cop.rb:54:in `inherited'
    # ./spec/rubocop/cop/rspec/cop_spec.rb:25:in `initialize'
    # ./spec/rubocop/cop/rspec/cop_spec.rb:25:in `new'
    # ./spec/rubocop/cop/rspec/cop_spec.rb:25:in `block (2 levels) in <top (required)>'
@pirj
Copy link
Member Author

@pirj pirj commented May 21, 2019

@bquorning @Darhazer Do you have something in mind that needs to be made here or is it ready to be merged?

Loading

@bquorning bquorning merged commit 5419d90 into rubocop:master May 21, 2019
11 checks passed
Loading
@bquorning
Copy link
Collaborator

@bquorning bquorning commented May 21, 2019

Nope, this is merge-able. Thank you for this cop ❤️

Loading

@pirj pirj deleted the add-leaky-constant-declaration-cop branch May 21, 2019
@dgollahon
Copy link
Contributor

@dgollahon dgollahon commented Jun 18, 2019

@pirj Sorry I didn't get a chance to review this. I haven't been doing much OSS lately and the ping got lost in my notification queue. Glad you pushed through and implemented the cop, I think it's a valuable one :)

I just skimmed my old comments so I don't know if I clearly explained this before or if it has a place in the good/bad examples in the docs (or maybe in the style guide?) is that cases like:

before do
  stub_const('Foo', Class.new)
  class Foo
    # define the class now
  end
end

that could be written to pass the cop like this:

before do
  fake_class = Class.new do
    # define the class now
  end
  stub_const('Foo', fake_class)
end

should, imo, usually be written like this:

let(:foo_class) do
  Class.new do
    # define the class now
  end
end

This way nothing mutates the constant space and you can't possibly run into the normal issues you can with stub_const. Usually when you need a faux class you don't need it to be referenced as a particular constant name. In rare cases where that is needed, I find it's usually overriding some existing constant (occasionally necessary in weird situations), or dynamically setting a constant on a newly generated dynamic class (and in this case i would use my let version on the outermost class, still not polluting the global constant space). My main point is that it's sometimes worth reminding people that classes are just objects and don't have to be assigned to a constant, so this strategy might have a place in our guide or documentation somewhere, but as with most techniques, it should be used judiciously.

As an aside / trivia, if you use this it helps to be aware that anonymous classes have unusual #name behavior.

Class.new.name # => nil

but if you write it this way

Foo = Class.new
Foo.name # => 'Foo'

the constant assignment causes the anonymous class to receive a name (i'm guessing this is some sort of const assignment hook that mutates the anonymous class, but I don't know exactly how it works, mechanically). In practice this makes it seem the same as if you had written your class the standard way

class Foo
end

Anyway, nice work.

Loading

@dgollahon
Copy link
Contributor

@dgollahon dgollahon commented Jun 18, 2019

Ah, @bquorning also pointed to a good alternate explanation of what I was saying here: #197 (comment)

Loading

@pirj
Copy link
Member Author

@pirj pirj commented Jun 18, 2019

@dgollahon, love the reasoning for sacrificial classes.

I had a problem similar to what you mention just today:

    let(:dummy_job) do
      Class.new(ActiveJob::Base) do
        def self.name
          'DummyJob'
        end
 
        def perform
          # ...
        end
      end
    end

I ended up adding:

    before do
      stub_const('DummyJob', dummy_job_class)
    end

simply because ActiveJob is expecting ::DummyJob to be present in integration tests.

So, YMMV.
Also, this case won't work, since parent class's def self.inherited expects to have a class name, the situation you mentioned.

Leaking constants problem is undeservedly missed from the guide.

Are you up to add sacrificial classes guideline to the style guide and update LeakyConstantDeclaration cop's good examples to reflect this?

Loading

@dgollahon
Copy link
Contributor

@dgollahon dgollahon commented Jun 18, 2019

because ActiveJob is expecting ::DummyJob to be present in integration tests

Really? Wow, that's odd. I haven't interacted with ActiveJob in a long time but that is a weird API.

Also, this case won't work, since parent class's def self.inherited expects to have a class name, the situation you mentioned.

Ah, yeah, interesting case. Any time you have inheritance hooks things get weird.

Are you up to add sacrificial classes guideline to the style guide and update LeakyConstantDeclaration cop's good examples to reflect this?

Maybe, but likely not. I'm a little less keen on maintaining the style guide v.s. rubocop-rspec, but I'm happy to weigh in on and review any changes if someone else would like to update the information. I might change my mind, but there are other things I'd probably rather tackle first.

Loading

pirj added a commit to pirj/rubocop-rspec that referenced this issue Jun 20, 2019
As mentioned here
rubocop#765 (comment),
it's not even necessary in some cases to stub a constant, a so-called
"sacrificial class" will do.
Originally mentioned article
http://blog.bitwrangler.com/2016/11/10/sacrificial-test-classes.html in
this comment rubocop#197 (comment)
pirj added a commit to pirj/rubocop-rspec that referenced this issue Jun 20, 2019
As mentioned here
rubocop#765 (comment),
it's not even necessary in some cases to stub a constant, a so-called
"sacrificial class" will do.
Originally mentioned article
http://blog.bitwrangler.com/2016/11/10/sacrificial-test-classes.html in
this comment rubocop#197 (comment)
pirj added a commit to pirj/rubocop-rspec that referenced this issue Jun 23, 2019
As mentioned here
rubocop#765 (comment),
it's not even necessary in some cases to stub a constant, a so-called
"sacrificial class" will do.
Originally mentioned article
http://blog.bitwrangler.com/2016/11/10/sacrificial-test-classes.html in
this comment rubocop#197 (comment)
pirj added a commit to pirj/rubocop-rspec that referenced this issue Jun 23, 2019
As mentioned here
rubocop#765 (comment),
it's not even necessary in some cases to stub a constant, a so-called
"sacrificial class" will do.
Originally mentioned article
http://blog.bitwrangler.com/2016/11/10/sacrificial-test-classes.html in
this comment rubocop#197 (comment)
pirj added a commit to pirj/rubocop-rspec that referenced this issue Jun 28, 2019
As mentioned here
rubocop#765 (comment),
it's not even necessary in some cases to stub a constant, a so-called
"sacrificial class" will do.
Originally mentioned article
http://blog.bitwrangler.com/2016/11/10/sacrificial-test-classes.html in
this comment rubocop#197 (comment)
pirj added a commit to pirj/rubocop-rspec that referenced this issue Jun 28, 2019
As mentioned here
rubocop#765 (comment),
it's not even necessary in some cases to stub a constant, a so-called
"sacrificial class" will do.
Originally mentioned article
http://blog.bitwrangler.com/2016/11/10/sacrificial-test-classes.html in
this comment rubocop#197 (comment)
@pirj
Copy link
Member Author

@pirj pirj commented Aug 4, 2019

@dgollahon In response to this #750 (comment)

It's actually a very good point that re-opening the class doesn't by itself lead to constant leakage.

I'm concerned about the possibility of the leakage when stub_const is being removed. Some cases of that are impossible to detect statically, e.g. when stub_const is defined in a shared example residing in another file.
This makes it a really hard task to detect if it's re-opening of a stubbed class or a new class definition and with all the downsides of the latter and existing workaround (Class.new do) for the former I'm not sure if it is worth it to allow re-opening case.

At the same time, I can think of a case when constants need to be defined in that stubbed class, e.g.

let(:klass) do
  Class.new(ClassUnderTest) do
    SOME_CONSTANT = ...
  end
end

and without re-opening, this example may become quite clumsy.

Let's give it another thought if we notice or someone complains that the cop doesn't simplify our lives as it is, what do you think?

Loading

kellysutton added a commit to kellysutton/rubocop-rspec that referenced this issue Oct 28, 2019
As mentioned here
rubocop#765 (comment),
it's not even necessary in some cases to stub a constant, a so-called
"sacrificial class" will do.
Originally mentioned article
http://blog.bitwrangler.com/2016/11/10/sacrificial-test-classes.html in
this comment rubocop#197 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants