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

Rails/Exit cop not catching all uses of exit? #1274

Open
nestor-custodio opened this issue May 8, 2024 · 10 comments
Open

Rails/Exit cop not catching all uses of exit? #1274

nestor-custodio opened this issue May 8, 2024 · 10 comments

Comments

@nestor-custodio
Copy link

nestor-custodio commented May 8, 2024

Preamble: Spent an entire day tracking down a CI issue that came down to an exit in a Rails task. That seemed to me like the kind of thing 'rubocop-rails' should catch, so I popped in here to make a feature request only to realize (as I was doing my pre-issue due diligence) that there is a cop for it and it just didn't catch this for whatever reason.

The Problem: An exit in a Rails task went unreported by RuboCop. The project's RuboCop config has no exceptions for this rule, nor is the path ignored.

Offending Code:

~/dev $vars rubocop
RUBOCOP_OPTS="--format='simple' --format='offenses' --color"
# ".rubocop.yml"

require:
  -⸱rubocop-faker
  -⸱rubocop-rails

AllCops:
  TargetRubyVersion:⸱3.2
  TargetRailsVersion:⸱6.1
  NewCops:⸱enable
  SuggestExtensions:⸱false

Style/FrozenStringLiteralComment:
  # Enforces use of the frozen_string_literal comment in every file.
  Enabled: false
# "lib/tasks/weekly_update.rake"

namespace :weekly do
  desc '...'
  task update: [:environment, :logs] do
    exit unless Date.current.sunday?

    # ...
  end
end

Expected behavior

A RuboCop error from the Rails/Exit cop for file "lib/tasks/weekly_update.rake" on the exit line.

Actual behavior

~/dev $rubocop lib/tasks/weekly_update.rake

1 file inspected, no offenses detected

--
0  Total in 0 files

Steps to reproduce the problem

Offending code (and config) is provided above.

RuboCop version

Include the output of rubocop -V or bundle exec rubocop -V if using Bundler. Here's an example:
You can see extension cop versions (e.g. rubocop-rails, rubocop-performance, and others) output by rubocop -V,
include them as well. Here's an example:

~/dev $bundle exec rubocop -V
1.63.4 (using Parser 3.3.1.0, rubocop-ast 1.31.3, running on ruby 3.2.2) +server [x86_64-linux]
  - rubocop-faker 1.1.0
  - rubocop-rails 2.24.1
@Earlopain
Copy link
Contributor

This cop by default exclude rake tasks. You can probably work around this by adding it to the includes yourself (make sure to use the proper inherit_mode).

I guess, what would the difference be when you use return instead? My understanding is that you for sure don't want exit in your rails code, where it brings down your puma worker or test runner but I don't really see a difference for a rake task

@nestor-custodio
Copy link
Author

A return is how we ultimately corrected this. I didn't realize this specific rule has an exclude directive for "lib/**/*.rake". Guess that explains it. Yikes.

Closing this as it's apparently intended behaviour (which seems yikes-y, honestly). 😬

@Earlopain
Copy link
Contributor

Cany you explain what is yikes here? To me there doesn't seem to be much difference between exit and return in that context.

The behaviour has been so since the cops introduction. I looked at the PR and issue for it but didn't find anything rationalizing why that folder is excluded. If there is a good case for it to run over rake tasks, I don't see why it can't change to do so.

@nestor-custodio
Copy link
Author

nestor-custodio commented Jun 10, 2024

If your test suite includes rake tasks, a stray exit there may bail out of the test suite prematurely. Since exit generates a 0 exit code by default, any CI automation will think your test suite has passed without issues, when the reality is any number of tests that might've failed never got to run at all.

IMO, since any exit could almost as easily become a return (or possible a raise if it's too deeply nested to come back cleanly), and the exit could break testing/CI tooling, "lib/**/*.rake" shouldn't be excluded by default here.

@Earlopain
Copy link
Contributor

That sounds like a pretty valid reason to me and perfectly in line for what this cop is supposed to do, per its own documentation

... to avoid exits during unit testing or running in production

So, feel free to reopen.

Though I'm curious. I'm not very well-versed in how rake tasks integrate with running tests in rails: how would I write a rake task in such a way that it is getting executed during my test suite (in a way that exit is harmful)?

@nestor-custodio
Copy link
Author

(This appears to have been closed prematurely. M'bad.)

@nestor-custodio
Copy link
Author

Ideally, your Rake task is completely oblivious to how the test suite operates, and instead the test itself will stub certain class/instance method calls and/or create fixtures for any data sources to be accessed, then the task is triggered via Rake::Task[task_name].invoke so you can afterwards verify certain method calls took place, and possibly check for expected side-effects (database updates, emails sent, etc).

@Earlopain
Copy link
Contributor

Thinking about this a bit more I'm not so sure anymore. Rails defines a few rake tasks, and these do explicitly fail with exit(1) or other status codes. I'd think that was the reason for not running this cop on rake files.

I can see reasoning for both sides, though I'm a lot less certain now and would err on the side of caution for this cop (i.e. how it is currently).

@nestor-custodio
Copy link
Author

That's fair, and I'd argue Rails sets an example by writing Rake tasks that fail with a non-0 exit code (which does not break test/CI tooling). Perhaps the issue isn't that the cop doesn't run on Rake tasks, but that if a Rake task is going to bail prematurely, it should do so with either a return (on success) or via exit with a non-0 param (on failure), as exit by itself (or exit 0) is problematic.

Not looking to push my opinion, however, and I'll accept your call on this one way or the other.

@Earlopain
Copy link
Contributor

Well, depending on how you write your rake tasks you definitly don't want to write exit but there are also valid use-cases where you do. Disallowing exit without an exit code only in rake tasks doesn't sound very consistent to me. If I use exit(1) it also makes sense to use exit.

For me it sounds good to leave this open in case someone else has opinions on this. Maybe testing frameworks can be improved to warn on early exits like this but I'm not sure if that is even possible with ruby.

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

No branches or pull requests

2 participants