`Rails/Exit` cop not catching all uses of `exit`?

[nestor-custodio]

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]

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]

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]

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]

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]

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]

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

[nestor-custodio]

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]

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]

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]

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.