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

Tests using AssertOffense should Fail with a usable message if the relevant cop is not defined #314

Conversation

brandoncc
Copy link
Contributor

@brandoncc brandoncc commented Aug 12, 2024

Including the AssertOffense module currently fails with a cryptic message if the cop can't be found. This PR introduces an explicit failure with a useful message, replacing the NoMethodError that is currently raised in this situation.

Before

Error:
RuboCop::Cop::MyCops::MyNewCopTest#test_works:
NoMethodError: undefined method `[]=' for nil
    /Users/brandoncc/.gem/ruby/3.3.1/gems/rubocop-minitest-0.35.1/lib/rubocop/minitest/assert_offense.rb:109:in `assert_offense'
    test/rubocop/cop/my_cops/my_new_cop_test.rb:12:in `block in <class:MyNewCopTest>'

After

Error:
RuboCop::Cop::MyCops::MyNewCopTest#test_works:
RuntimeError: Cop not defined: RuboCop::Cop::MyCops::MyNewCop
    /Users/brandoncc/dev/rubocop-minitest/lib/rubocop/minitest/assert_offense.rb:81:in `cop'
    /Users/brandoncc/dev/rubocop-minitest/lib/rubocop/minitest/assert_offense.rb:111:in `assert_offense'
    test/rubocop/cop/my_cops/my_new_cop_test.rb:12:in `block in <class:MyNewCopTest>'

My first implementation simply changed AssertOffense#setup to fail if the cop wasn't found, but that broke four tests in the test suite. Given that the advice given is to require support.rb, which includes AssertOffense on every instance of Minitest::Test, I realized that implementation was likely to break many gem-consumer repositories in this same way.

The second implementation refactors the initialization of a Cop to be lazy, so that the error is only raised when the cop is attempted to be accessed. This has the downside of @cop being nil until #cop is called, but these failures will only happen in tests where @cop is accessed directly. I believe these failures, if experienced by consumers of the gem, will be easier to resolve than the errors likely to have been introduced by my first implementation.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@brandoncc brandoncc force-pushed the brandoncc/fail-with-usable-message-if-cop-not-defined branch 2 times, most recently from f006b18 to 839792f Compare August 12, 2024 20:35
@brandoncc brandoncc marked this pull request as ready for review August 12, 2024 21:15
Currently, when a test class includes `AssertOffense`,
`AssertOffense#setup` attempts to dynamically identify the Cop under
test. If the Cop class cannot be found, `#setup` exits early.

In these cases, the test cases continue to execute with a nil `@cop`
since it wasn't set in `#setup` as expected. When helpers such as
`#assert_offense` are used, they access `@cop` and the tests can fail
with cryptic messages such as:

```
NoMethodError: undefined method `[]=' for nil
```

Initialization of the Cop was refactored to a lazily-called accessor
method which has the same caching functionality via `@cop`, but raises
an error if the expected Cop class isn't defined.
@brandoncc brandoncc force-pushed the brandoncc/fail-with-usable-message-if-cop-not-defined branch from 839792f to 2c2add0 Compare August 13, 2024 21:38
Comment on lines -109 to +110
@cop.instance_variable_get(:@options)[:autocorrect] = true
enable_autocorrect
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was forced to make this change because changing the instance variable to a method call caused method to have an AbcSize offense.

Comment on lines +78 to +79
def cop
@cop ||= begin
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the meat of the change, lazily-loading the cop in order to raise a nice error when the cop is used if the class isn't available.

@koic koic merged commit 859751a into rubocop:master Aug 14, 2024
14 checks passed
@koic
Copy link
Member

koic commented Aug 14, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants