RSpec/ExpectChange doesn't work well with nested class

[YusukeIwaki]

We use RSpec/ExpectChange with EnforcedStyle: block.

This rule works well with not nested class like below:

it 'should create new token' do
  expect { subject }.to change(Token, :count).by(1)
end

But when we use nested class like User::Token instead of Token, this rule doesn't work.
It can be easily reproduced with spec/rubocop/cop/rspec/expect_change_spec.rb.

Failures:

  1) RuboCop::Cop::RSpec::ExpectChange with EnforcedStyle `block` flags implicit block expectation syntax with nested class
     Failure/Error: super
     
       Diff:
       @@ -1,5 +1,4 @@
                it do
                  expect(run).to change(User::Token, :count).by(1)
       -                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `change { User::Token.count }`.
                end
       
     # ./spec/support/expect_offense.rb:13:in `expect_offense'
     # ./spec/rubocop/cop/rspec/expect_change_spec.rb:103:in `block (3 levels) in <top (required)>'

[YusukeIwaki]

ruby-parse shows AST like below for expect { subject }.to change(User::Token, :count).by(1)

                (send
                  (block
                    (send nil :expect)
                    (args)
                    (send nil :subject)) :to
                  (send
                    (send nil :change
                      (const
                        (const nil :User) :Token)
                      (sym :count)) :by
                    (int 1))))))))

RSpec::ExpectChange checks the AST with regex (send nil? :change ({const send} nil? $_) (sym $_)), however it doesn't match with

                      (const
                        (const nil :User) :Token)

[pirj]

I see what you mean for ExpectChange, it's missing if the constant is referenced to as a top-level constant, e.g. ::User, and a namespaced constant when the EnforcedStyle is block.

It seems to be relatively easy to fix:

        def_node_matcher :expect_change_with_arguments, <<-PATTERN
-          (send nil? :change ({const send} nil? $_) (sym $_))
+          (send nil? :change {$const (send nil? $_)} (sym $_))
        PATTERN

        def_node_matcher :expect_change_with_block, <<-PATTERN
          (block
            (send nil? :change)
            (args)
-            (send ({const send} nil? $_) $_)
+            (send {$const (send nil? $_)} $_)
          )
        PATTERN

plus

          expect_change_with_arguments(node) do |receiver, message|
-            msg = format(MSG_CALL, obj: receiver, attr: message)
+            msg = format(MSG_CALL, obj: receiver.source, attr: message)
            add_offense(node, message: msg) do |corrector|
-              replacement = "change { #{receiver}.#{message} }"
+              replacement = "change { #{receiver.source}.#{message} }"
              corrector.replace(node, replacement)
            end
          end
@ lib/rubocop/cop/rspec/expect_change.rb:67 @ module RuboCop
          return unless style == :method_call

          expect_change_with_block(node) do |receiver, message|
-            msg = format(MSG_BLOCK, obj: receiver, attr: message)
+            msg = format(MSG_BLOCK, obj: receiver.source, attr: message)
            add_offense(node, message: msg) do |corrector|
-              replacement = "change(#{receiver}, :#{message})"
+              replacement = "change(#{receiver.source}, :#{message})"
              corrector.replace(node, replacement)
            end
          end

A completely different thing is expect(run).to change. It is an implicit block expectation syntax that will be deprecated by RSpec, and eventually removed.

More on this #814 rspec/rspec-expectations#1139 rspec/rspec-expectations#1125 https://blog.rubystyle.guide/rspec/2019/07/17/rspec-implicit-block-syntax.html

There's a dedicated cop, RSpec/ImplicitBlockExpectation specificaly to catch those usages #789, but now, looking at your code our spec code I see that it fails to detect expect(run) and is only capable of detecting is_expected.

ExpectChange seems to do the job that ImplicitBlockExpectation is supposed to.
That is yet another thing that might be improved.

Would you like to tackle those issues, @YusukeIwaki ?

[pirj]

Would you like to turn the changes from the above comment into a pull request and throw in a couple of tests, @YusukeIwaki ?

[YusukeIwaki]

@pirj Thank you for your kind advice, I will try it.