An error occurred while RSpec/ChangeByZero cop was inspecting #1983

danielduan · 2024-10-23T07:58:14Z

Rubocop Rspec throws errors when the following line of code is inspected. I believe the AST tree does not take this code pattern into account:

expect do
    post "/v1/verify", params: {
        'param1': param1,
        'param2': param2,
    }, as: :json
end.not_to change(ClassA, :count) and change(ClassB, :count).by(0)

We have multiple instances of this pattern and it errors on the last end.not_to ... and ...

end.to change(ClassA, :count).by(1) and change(ClassB, :count).by(0)

Expected behavior

This code is semantically correct so it should the AST should be able to crawl this code correctly.

Actual behavior

Describe here what actually happened.

undefined method `method_name' for an instance of RuboCop::AST::AndNode
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-rspec-3.1.0/lib/rubocop/cop/rspec/change_by_zero.rb:121:in `compound_expectations?'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-rspec-3.1.0/lib/rubocop/cop/rspec/change_by_zero.rb:106:in `register_offense'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-rspec-3.1.0/lib/rubocop/cop/rspec/change_by_zero.rb:94:in `block in on_send'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-rspec-3.1.0/lib/rubocop/cop/rspec/change_by_zero.rb:85:in `expect_change_with_arguments'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-rspec-3.1.0/lib/rubocop/cop/rspec/change_by_zero.rb:93:in `on_send'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:143:in `public_send'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:143:in `block (2 levels) in trigger_restricted_cops'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:171:in `with_cop_error_handling'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:142:in `block in trigger_restricted_cops'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:141:in `each'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:141:in `trigger_restricted_cops'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:70:in `on_send'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:159:in `block in on_send'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:156:in `each'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:156:in `each_with_index'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:156:in `on_send'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:71:in `on_send'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:137:in `block in on_dstr'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:137:in `each'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:137:in `on_dstr'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:71:in `on_and'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:137:in `block in on_dstr'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:137:in `each'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:137:in `on_dstr'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:71:in `on_begin'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:158:in `on_block'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:71:in `on_block'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:158:in `on_block'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:71:in `on_block'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:137:in `block in on_dstr'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:137:in `each'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:137:in `on_dstr'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:71:in `on_begin'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:158:in `on_block'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:71:in `on_block'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:137:in `block in on_dstr'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:137:in `each'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:137:in `on_dstr'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:71:in `on_begin'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:158:in `on_block'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:71:in `on_block'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:137:in `block in on_dstr'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:137:in `each'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:137:in `on_dstr'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:71:in `on_begin'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-ast-1.32.3/lib/rubocop/ast/traversal.rb:20:in `walk'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/commissioner.rb:87:in `investigate'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/team.rb:168:in `investigate_partial'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cop/team.rb:102:in `investigate'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:349:in `block in inspect_file'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:348:in `each'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:348:in `flat_map'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:348:in `inspect_file'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:291:in `block in do_inspection_loop'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:325:in `block in iterate_until_no_changes'
<internal:kernel>:187:in `loop'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:318:in `iterate_until_no_changes'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:287:in `do_inspection_loop'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:168:in `block in file_offenses'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:193:in `file_offense_cache'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:167:in `file_offenses'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:158:in `process_file'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:139:in `block in each_inspected_file'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:138:in `each'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:138:in `reduce'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:138:in `each_inspected_file'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:124:in `inspect_files'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/runner.rb:77:in `run'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cli/command/execute_runner.rb:26:in `block in execute_runner'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cli/command/execute_runner.rb:52:in `with_redirect'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cli/command/execute_runner.rb:25:in `execute_runner'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cli/command/execute_runner.rb:17:in `run'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cli/command.rb:11:in `run'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cli/environment.rb:18:in `run'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cli.rb:122:in `run_command'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cli.rb:129:in `execute_runners'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cli.rb:51:in `block in run'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cli.rb:81:in `profile_if_needed'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/lib/rubocop/cli.rb:43:in `run'
/Users/danielduan/.rvm/gems/ruby-3.3.5/gems/rubocop-1.66.1/exe/rubocop:19:in `<top (required)>'
/Users/danielduan/.rvm/gems/ruby-3.3.5/bin/rubocop:25:in `load'
/Users/danielduan/.rvm/gems/ruby-3.3.5/bin/rubocop:25:in `<main>'
/Users/danielduan/.rvm/gems/ruby-3.3.5/bin/ruby_executable_hooks:22:in `eval'
/Users/danielduan/.rvm/gems/ruby-3.3.5/bin/ruby_executable_hooks:22:in `<main>'

Steps to reproduce the problem

Please see the code block above.

It was autocorrected from:

end.to change(ClassA, :count).by(0) and change(ClassB, :count).by(0)

RuboCop version

$ [bundle exec] rubocop -V
1.66.1 (using Parser 3.3.5.0, rubocop-ast 1.32.3, analyzing as Ruby 3.3, running on ruby 3.3.5) [arm64-mac]
   rubocop-factory_bot (2.26.1)
      rubocop (~> 1.61)
    rubocop-md (1.2.4)
      rubocop (>= 1.45)
    rubocop-migration (0.5.0)
      activesupport
      rubocop (>= 1.34)
      rubocop-rails
    rubocop-minitest (0.36.0)
      rubocop (>= 1.61, < 2.0)
      rubocop-ast (>= 1.31.1, < 2.0)
    rubocop-packaging (0.5.2)
      rubocop (>= 1.33, < 2.0)
    rubocop-performance (1.22.1)
      rubocop (>= 1.48.1, < 2.0)
      rubocop-ast (>= 1.31.1, < 2.0)
    rubocop-rails_config (1.16.0)
      rubocop (>= 1.57.0)
      rubocop-ast (>= 1.26.0)
      rubocop-md
      rubocop-minitest (~> 0.22)
      rubocop-packaging (~> 0.5)
      rubocop-performance (~> 1.11)
      rubocop-rails (~> 2.0)
    rubocop-rspec (3.1.0)
      rubocop (~> 1.61)
    rubocop-rspec_rails (2.30.0)
      rubocop (~> 1.61)
      rubocop-rspec (~> 3, >= 3.0.1)

The text was updated successfully, but these errors were encountered:

… `change (...)`, concatenated with `and` and `or` fix: #1983

pirj · 2024-10-23T15:57:55Z

First of all, not_to matcher1.and is restricted and causes an error in RSpec. https://github.com/rspec/rspec-expectations/blob/838952dc1416c943e7933684c47249a77481e7f5/lib/rspec/matchers/built_in/compound.rb#L17

And, most importantly, shouldn’t this be

- change(ClassA, :count) and
+ change(ClassA, :count).and

Are you certain that the second matcher ever gets a matches? call?

danielduan · 2024-10-23T16:29:45Z

First of all, not_to matcher1.and is restricted and causes an error in RSpec. https://github.com/rspec/rspec-expectations/blob/838952dc1416c943e7933684c47249a77481e7f5/lib/rspec/matchers/built_in/compound.rb#L17

And, most importantly, shouldn’t this be
- change(ClassA, :count) and
+ change(ClassA, :count).and
Are you certain that the second matcher ever gets a matches? call?

Not certain at all. It was autocorrected as mentioned above. This should probably be disallowed in one of the rspec rules.

pirj · 2024-10-23T17:32:14Z

Would you agree that this should be fixed on your side, and this cop does not necessarily have to consider this, @danielduan ?

It might be an idea for another cop. Because, frankly, I was puzzled by this, wondering if this works or not with no doubt that I have never seen this syntax.

What troubles me thought is that and do_something became a completely legitimate approach in Ruby.
Since and change is effectively that, why does the cop error out on that? Will it on inline if that/unless this, too?

danielduan · 2024-10-24T02:19:56Z

@pirj I do think cop should not throw errors on any valid syntax, which it looks like is being fixed and released. Whether we need a cop to separately manage the preference for and against this syntax format is up to you all.

pirj · 2024-10-24T06:24:47Z

There is a distinction between a valid Ruby syntax, and sensible usage of RSpec DSL.

I’d like to highlight that even though we’ve fixed the cop to tolerate valid Ruby syntax, we didn’t fully did that (if and unless eg would still fail), but also now it conceals a nonsensical RSpec constuct.

So, with that fix, we helped you and potentially other future users to write RSpec code that doesn’t do what it looks like doing.

There is no cop to detect this. Before it’s created, I would rather have the existing cop to error out, and users to find this issue and having an idea that they have to fix their specs rather than living with deceitful specs until rubocop-rspec 3.0 where we enable new pending cops.

koic transferred this issue from rubocop/rubocop Oct 23, 2024

ydah added the bug label Oct 23, 2024

ydah added a commit that referenced this issue Oct 23, 2024

Fix an error for RSpec/ChangeByZero when change (...) .by (0) and…

130c564

… `change (...)`, concatenated with `and` and `or` fix: #1983

ydah mentioned this issue Oct 23, 2024

Fix an error for RSpec/ChangeByZero when change (...) .by (0) and change (...), concatenated with and and or #1984

Merged

5 tasks

ydah closed this as completed in #1984 Oct 23, 2024

pirj mentioned this issue Oct 25, 2024

Revert an change that would conceal unintentional RSpec syntax #1985

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

An error occurred while RSpec/ChangeByZero cop was inspecting #1983

An error occurred while RSpec/ChangeByZero cop was inspecting #1983

danielduan commented Oct 23, 2024 •

edited

Loading

pirj commented Oct 23, 2024

danielduan commented Oct 23, 2024 •

edited

Loading

pirj commented Oct 23, 2024

danielduan commented Oct 24, 2024 •

edited

Loading

pirj commented Oct 24, 2024

An error occurred while RSpec/ChangeByZero cop was inspecting #1983

An error occurred while RSpec/ChangeByZero cop was inspecting #1983

Comments

danielduan commented Oct 23, 2024 • edited Loading

Expected behavior

Actual behavior

Steps to reproduce the problem

RuboCop version

pirj commented Oct 23, 2024

danielduan commented Oct 23, 2024 • edited Loading

pirj commented Oct 23, 2024

danielduan commented Oct 24, 2024 • edited Loading

pirj commented Oct 24, 2024

danielduan commented Oct 23, 2024 •

edited

Loading

danielduan commented Oct 23, 2024 •

edited

Loading

danielduan commented Oct 24, 2024 •

edited

Loading