Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# main [(unreleased)](https://github.com/fastruby/next_rails/compare/v1.6.0...main)

- [BUGFIX: example](https://github.com/fastruby/next_rails/pull/<number>)
- [FEATURE: Validate the DeprecationTracker mode at initialization, treating a blank mode as the default `save`](https://github.com/fastruby/next_rails/pull/186)

* Your changes/patches go here.

Expand Down
6 changes: 5 additions & 1 deletion exe/deprecations
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,14 @@ require "json"
require "rainbow"
require "optparse"
require "set"
require_relative "../lib/deprecation_tracker/valid_modes"
require_relative "../lib/deprecation_tracker/shard_merger"

def run_tests(deprecation_warnings, opts = {})
tracker_mode = opts[:tracker_mode]
unless DeprecationTracker.valid_mode?(tracker_mode)
abort "Invalid --tracker-mode #{tracker_mode.inspect}. Must be one of: #{DeprecationTracker.valid_modes_display}."
end
Comment on lines 10 to +13
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this guard skip the case where no --tracker-mode is passed? --tracker-mode is optional, so on the documented default deprecations run (README + the help examples) opts[:tracker_mode] is nil, and valid_mode?(nil) is falsey, so this aborts with Invalid --tracker-mode nil. before reaching rspec.

I cloned the branch and confirmed it: deprecations run (no --tracker-mode) exits 1 on this branch, whereas on main it falls through to DEPRECATION_TRACKER= bundle exec rspec ... and the library treats the blank value as the default save. That blank-as-default path is exactly what sanitize_mode adds for the library, but the CLI guard rejects nil/blank before that logic runs.

What about only validating when a mode was actually supplied, so a genuine typo (--tracker-mode bogus) still fails loudly but the default keeps working?

Suggested change
tracker_mode = opts[:tracker_mode]
unless DeprecationTracker.valid_mode?(tracker_mode)
abort "Invalid --tracker-mode #{tracker_mode.inspect}. Must be one of: #{DeprecationTracker.valid_modes_display}."
end
tracker_mode = DeprecationTracker.sanitize_mode(opts[:tracker_mode])
if tracker_mode && !DeprecationTracker.valid_mode?(tracker_mode)
abort "Invalid --tracker-mode #{tracker_mode.inspect}. Must be one of: #{DeprecationTracker.valid_modes_display}."
end

next_mode = opts[:next_mode]
rspec_command = if next_mode
"bin/next rspec"
Expand Down Expand Up @@ -69,7 +73,7 @@ option_parser = OptionParser.new do |opts|
options[:next] = next_mode
end

opts.on("--tracker-mode MODE", "Set DEPRECATION_TRACKER in test mode. Options: save or compare") do |tracker_mode|
opts.on("--tracker-mode MODE", "Set DEPRECATION_TRACKER in test mode. Options: #{DeprecationTracker.valid_modes_display}") do |tracker_mode|
options[:tracker_mode] = tracker_mode
end

Expand Down
17 changes: 16 additions & 1 deletion lib/deprecation_tracker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
# Tracks deprecation warnings, grouped by spec file. After the test run, compare against shitlist of expected
# deprecation warnings. If anything is added or removed, raise an error with a diff of the changes.
#
require_relative "deprecation_tracker/valid_modes"

class DeprecationTracker
UnexpectedDeprecations = Class.new(StandardError)

Expand Down Expand Up @@ -75,9 +77,19 @@ def Kernel.warn(*args, &block)

DEFAULT_PATH = "spec/support/deprecation_warning.shitlist.json"

# Returns the mode as-is, or nil when it is blank. A blank DEPRECATION_TRACKER
# (e.g. `DEPRECATION_TRACKER= rspec`) is truthy in Ruby, so callers can use this
# to treat an empty value as unset and fall back to the default mode.
def self.sanitize_mode(mode)
return if mode.nil?

stripped = mode.to_s.strip
stripped.empty? ? nil : stripped
end

def self.init_tracker(opts = {})
shitlist_path = opts[:shitlist_path] || DEFAULT_PATH
mode = opts[:mode] || ENV["DEPRECATION_TRACKER"] || :save
mode = sanitize_mode(opts[:mode] || ENV["DEPRECATION_TRACKER"]) || :save
transform_message = opts[:transform_message]
node_index = opts[:node_index]
deprecation_tracker = DeprecationTracker.new(shitlist_path, transform_message, mode, node_index: node_index)
Expand Down Expand Up @@ -141,6 +153,9 @@ def initialize(shitlist_path, transform_message = nil, mode = :save, node_index:
@transform_message = transform_message || -> (message) { message }
@deprecation_messages = {}
@mode = mode ? mode.to_sym : :save
unless self.class.valid_mode?(@mode)
raise ArgumentError, "mode must be one of: #{self.class.valid_modes_display}. Got: #{mode.inspect}"
end
if @mode == :compare && node_index
raise ArgumentError, "node_index cannot be used with compare mode"
end
Expand Down
12 changes: 12 additions & 0 deletions lib/deprecation_tracker/valid_modes.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
class DeprecationTracker
VALID_MODES = %i[save compare].freeze

# Human-readable list of valid modes for error messages and CLI help text.
def self.valid_modes_display
@valid_modes_display ||= VALID_MODES.map(&:to_s).join(", ")
end

def self.valid_mode?(mode)
mode && VALID_MODES.include?(mode.to_sym)
end
end
62 changes: 55 additions & 7 deletions spec/deprecation_tracker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -263,13 +263,6 @@
tracker = DeprecationTracker.new(shitlist_path, nil, nil)
expect(tracker.mode).to eq(:save)
end

it "does not save nor compare if mode is invalid" do
Comment thread
JuanVqz marked this conversation as resolved.
tracker = DeprecationTracker.new(shitlist_path, nil, "random_stuff")
expect(tracker).not_to receive(:save)
expect(tracker).not_to receive(:compare)
tracker.after_run
end
end

describe "#init_tracker" do
Expand Down Expand Up @@ -426,6 +419,61 @@ def self.behavior
tracker = DeprecationTracker.init_tracker({})
expect(tracker.mode).to eq(:compare)
end

it "raises ArgumentError naming the invalid ENV['DEPRECATION_TRACKER'] value" do
stub_const("ENV", ENV.to_h.merge("DEPRECATION_TRACKER" => "bogus"))
expect {
DeprecationTracker.init_tracker({})
}.to raise_error(ArgumentError, /mode must be one of: save, compare\..*bogus/)
end

it "treats a blank ENV['DEPRECATION_TRACKER'] as unset and defaults to save" do
stub_const("ENV", ENV.to_h.merge("DEPRECATION_TRACKER" => ""))
tracker = DeprecationTracker.init_tracker({})
expect(tracker.mode).to eq(:save)
end

it "treats a blank opts[:mode] as unset and defaults to save" do
tracker = DeprecationTracker.init_tracker(mode: "")
expect(tracker.mode).to eq(:save)
end
end

describe "VALID_MODES" do
it "accepts all valid modes without error" do
DeprecationTracker::VALID_MODES.each do |mode|
expect {
DeprecationTracker.new(shitlist_path, nil, mode)
}.not_to raise_error
end
end

it "rejects an invalid mode at construction, naming the bad value" do
expect {
DeprecationTracker.new(shitlist_path, nil, "random_stuff")
}.to raise_error(ArgumentError, /mode must be one of: save, compare\..*random_stuff/)
end
end

describe ".valid_mode?" do
it "returns true for valid modes, as symbol or string" do
expect(DeprecationTracker.valid_mode?(:save)).to be_truthy
expect(DeprecationTracker.valid_mode?("compare")).to be_truthy
end

it "returns a falsey value for an unknown mode" do
expect(DeprecationTracker.valid_mode?("random_stuff")).to be_falsey
end

it "returns a falsey value for nil" do
expect(DeprecationTracker.valid_mode?(nil)).to be_falsey
end
end

describe ".valid_modes_display" do
it "renders the valid modes as a comma-separated string" do
expect(DeprecationTracker.valid_modes_display).to eq("save, compare")
end
end

describe DeprecationTracker::KernelWarnTracker do
Expand Down
Loading