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

[Fix #301] Add new Minitest/Focus cop #302

Merged
merged 1 commit into from
Feb 17, 2024
Merged

Conversation

jaredmoody
Copy link
Contributor

@jaredmoody jaredmoody commented Feb 14, 2024

Checks for focused tests in the formats supported by minitest-focus.

I decided not to add an auto-corrector because IME it gets in the way when using editor auto-formatting. When focusing a test and then saving the file while working on the focused test, the focus gets removed, running everything.

However, I could add this if others disagree.

Hat tip to @NewAlexandria for the original PR.


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.

@NewAlexandria
Copy link

yea, thanks for the PR. I just have not had time to come back to it and sort out the tests.

@NewAlexandria
Copy link

is there a good guide or doc on write an auto-correct method? I'm not sure it's a good idea in this case, but I'd like to know.

IMO it may not be a good things to autocorrect because many, including me, leave -A on as part of save and commits. That would make it hard to use focus the way it's intended to for local testing. I guess, i.e., I'd want to make the autocorrection of focus only happen for specific environments.

@koic
Copy link
Member

koic commented Feb 15, 2024

The focus method that is not modifying the test method should not trigger warnings (e.g., calls to focus method alone). Can you add tests for this case and update the implementation accordingly?

@koic
Copy link
Member

koic commented Feb 15, 2024

I decided not to add an auto-corrector because IME it gets in the way when using editor auto-formatting. When focusing a test and then saving the file while working on the focused test, the focus gets removed, running everything.

While it has not been released yet, the next release of RuboCop will enable control through --editor-mode. Cops set with AutoCorrect: contextual will not autocorrect when --editor-mode is used.
rubocop/rubocop#12682

@jaredmoody
Copy link
Contributor Author

The focus method that is not modifying the test method should not trigger warnings (e.g., calls to focus method alone). Can you add tests for this case and update the implementation accordingly?

Per the README, minitest-focus allows focusing by directly - by placing the focus on the same line, or indirectly, on the previous line, which sets a "trap", focusing the next test. I added an additional "bad" example showing both focus possibilities.

While it has not been released yet, the next release of RuboCop will enable control through --editor-mode. Cops set with AutoCorrect: contextual will not autocorrect when --editor-mode is used.

This is cool! I think an autocorrect for this is lower value, since usually only a single test will be focused, but I'll take a stab at autocorrecting. Is there anything I need to do to set this cop to AutoCorrect: contextual?

@jaredmoody jaredmoody changed the title [Fix #301] Add new Minitest/NoFocus cop [Fix #301] Add new Minitest/Focus cop Feb 16, 2024
@jaredmoody
Copy link
Contributor Author

I've added auto-correction and tests are passing, but as I've never written one of these before, I don't know if I did it the best way, suggestions welcome.

@koic
Copy link
Member

koic commented Feb 16, 2024

Per the README, minitest-focus allows focusing by directly - by placing the focus on the same line, or indirectly, on the previous line, which sets a "trap", focusing the next test. I added an additional "bad" example showing both focus possibilities.

Oops! I was mistaken 🙇 You're correct!

Is there anything I need to do to set this cop to AutoCorrect: contextual?

Since AutoCorrect: contextual has not been released yet, it's fine to keep the current behavior of autocorrection as is. It can be addressed later once supported.

@koic koic merged commit 6994072 into rubocop:master Feb 17, 2024
13 checks passed
@koic
Copy link
Member

koic commented Feb 17, 2024

Thanks!

@jaredmoody jaredmoody deleted the focus branch February 17, 2024 06:36
koic added a commit to koic/rubocop-minitest that referenced this pull request Mar 1, 2024
koic added a commit to koic/rubocop-minitest that referenced this pull request Mar 1, 2024
Follow up rubocop#302 (comment).

This PR updates the dependency version to RuboCop 1.61, which supports `AutoCorrect: contextual`,
and updates `Minitest/Focus`'s config.
https://github.com/rubocop/rubocop/releases/tag/v1.61.0
koic added a commit to koic/rubocop-minitest that referenced this pull request Mar 1, 2024
Follow up rubocop#302 (comment).

This PR updates the dependency version to RuboCop 1.61, which supports `AutoCorrect: contextual`,
and updates `Minitest/Focus`'s config.
https://github.com/rubocop/rubocop/releases/tag/v1.61.0
koic added a commit to koic/rubocop-minitest that referenced this pull request Mar 1, 2024
Follow up rubocop#302 (comment).

This PR updates the dependency version to RuboCop 1.61, which supports `AutoCorrect: contextual`,
and updates `Minitest/Focus`'s config.
https://github.com/rubocop/rubocop/releases/tag/v1.61.0
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.

3 participants