From 3ee8822a1d7ecd3fb6a50ed4ea4ce95232cf6950 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20V=C3=A1squez?= Date: Mon, 18 May 2026 14:34:20 -0600 Subject: [PATCH 1/2] Add VALID_MODES constant and validate mode at initialization Centralizes valid tracker modes so adding/removing an option requires a single change. Raises ArgumentError early instead of silently no-oping on invalid input, covering both library and CLI entry points. --- exe/deprecations | 7 +++++-- lib/deprecation_tracker.rb | 4 ++++ spec/deprecation_tracker_spec.rb | 30 +++++++++++++++++++++++++----- 3 files changed, 34 insertions(+), 7 deletions(-) diff --git a/exe/deprecations b/exe/deprecations index db824d1..1cf7a1d 100755 --- a/exe/deprecations +++ b/exe/deprecations @@ -3,10 +3,13 @@ require "json" require "rainbow" require "optparse" require "set" -require_relative "../lib/deprecation_tracker/shard_merger" +require_relative "../lib/deprecation_tracker" def run_tests(deprecation_warnings, opts = {}) tracker_mode = opts[:tracker_mode] + if tracker_mode && !DeprecationTracker::VALID_MODES.include?(tracker_mode.to_sym) + abort "Invalid --tracker-mode #{tracker_mode.inspect}. Must be one of: #{DeprecationTracker::VALID_MODES.map(&:to_s).join(", ")}." + end next_mode = opts[:next_mode] rspec_command = if next_mode "bin/next rspec" @@ -69,7 +72,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.map(&:to_s).join(", ")}") do |tracker_mode| options[:tracker_mode] = tracker_mode end diff --git a/lib/deprecation_tracker.rb b/lib/deprecation_tracker.rb index 2c357be..3187a24 100644 --- a/lib/deprecation_tracker.rb +++ b/lib/deprecation_tracker.rb @@ -12,6 +12,7 @@ # class DeprecationTracker UnexpectedDeprecations = Class.new(StandardError) + VALID_MODES = %i[save compare].freeze module KernelWarnTracker def self.callbacks @@ -141,6 +142,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 VALID_MODES.include?(@mode) + raise ArgumentError, "mode must be one of #{VALID_MODES.map(&:to_s).join(", ")}, got: #{mode.inspect}" + end if @mode == :compare && node_index raise ArgumentError, "node_index cannot be used with compare mode" end diff --git a/spec/deprecation_tracker_spec.rb b/spec/deprecation_tracker_spec.rb index 67bb311..920de14 100644 --- a/spec/deprecation_tracker_spec.rb +++ b/spec/deprecation_tracker_spec.rb @@ -264,11 +264,10 @@ expect(tracker.mode).to eq(:save) end - it "does not save nor compare if mode is invalid" do - tracker = DeprecationTracker.new(shitlist_path, nil, "random_stuff") - expect(tracker).not_to receive(:save) - expect(tracker).not_to receive(:compare) - tracker.after_run + it "raises ArgumentError for invalid mode" do + expect { + DeprecationTracker.new(shitlist_path, nil, "random_stuff") + }.to raise_error(ArgumentError, /mode must be one of save, compare/) end end @@ -426,6 +425,27 @@ def self.behavior tracker = DeprecationTracker.init_tracker({}) expect(tracker.mode).to eq(:compare) end + + it "raises ArgumentError when ENV['DEPRECATION_TRACKER'] is invalid" 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/) + end + end + + describe "VALID_MODES" do + it "contains save and compare" do + expect(DeprecationTracker::VALID_MODES).to contain_exactly(:save, :compare) + end + + 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 end describe DeprecationTracker::KernelWarnTracker do From 9e70ca4f713c594aaca58c5ac88690cfcc59f2cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20V=C3=A1squez?= Date: Mon, 18 May 2026 15:32:24 -0600 Subject: [PATCH 2/2] Address code review feedback on valid modes refactor Extract constants to valid_modes.rb so exe/deprecations only loads the full lib when needed. Use VALID_MODES_DISPLAY to avoid repeated map/join. Loosen spec assertions to not couple on mode count or message wording. --- exe/deprecations | 7 ++++--- lib/deprecation_tracker.rb | 5 +++-- lib/deprecation_tracker/valid_modes.rb | 4 ++++ spec/deprecation_tracker_spec.rb | 6 +++--- 4 files changed, 14 insertions(+), 8 deletions(-) create mode 100644 lib/deprecation_tracker/valid_modes.rb diff --git a/exe/deprecations b/exe/deprecations index 1cf7a1d..aff4176 100755 --- a/exe/deprecations +++ b/exe/deprecations @@ -3,12 +3,13 @@ require "json" require "rainbow" require "optparse" require "set" -require_relative "../lib/deprecation_tracker" +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] if tracker_mode && !DeprecationTracker::VALID_MODES.include?(tracker_mode.to_sym) - abort "Invalid --tracker-mode #{tracker_mode.inspect}. Must be one of: #{DeprecationTracker::VALID_MODES.map(&:to_s).join(", ")}." + 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 @@ -72,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: #{DeprecationTracker::VALID_MODES.map(&:to_s).join(", ")}") 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 diff --git a/lib/deprecation_tracker.rb b/lib/deprecation_tracker.rb index 3187a24..f83ad27 100644 --- a/lib/deprecation_tracker.rb +++ b/lib/deprecation_tracker.rb @@ -10,9 +10,10 @@ # 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) - VALID_MODES = %i[save compare].freeze module KernelWarnTracker def self.callbacks @@ -143,7 +144,7 @@ def initialize(shitlist_path, transform_message = nil, mode = :save, node_index: @deprecation_messages = {} @mode = mode ? mode.to_sym : :save unless VALID_MODES.include?(@mode) - raise ArgumentError, "mode must be one of #{VALID_MODES.map(&:to_s).join(", ")}, got: #{mode.inspect}" + raise ArgumentError, "mode must be one of #{VALID_MODES_DISPLAY}, got: #{mode.inspect}" end if @mode == :compare && node_index raise ArgumentError, "node_index cannot be used with compare mode" diff --git a/lib/deprecation_tracker/valid_modes.rb b/lib/deprecation_tracker/valid_modes.rb new file mode 100644 index 0000000..7afe503 --- /dev/null +++ b/lib/deprecation_tracker/valid_modes.rb @@ -0,0 +1,4 @@ +class DeprecationTracker + VALID_MODES = %i[save compare].freeze + VALID_MODES_DISPLAY = VALID_MODES.map(&:to_s).join(", ").freeze +end diff --git a/spec/deprecation_tracker_spec.rb b/spec/deprecation_tracker_spec.rb index 920de14..9e0064c 100644 --- a/spec/deprecation_tracker_spec.rb +++ b/spec/deprecation_tracker_spec.rb @@ -267,7 +267,7 @@ it "raises ArgumentError for invalid mode" do expect { DeprecationTracker.new(shitlist_path, nil, "random_stuff") - }.to raise_error(ArgumentError, /mode must be one of save, compare/) + }.to raise_error(ArgumentError) end end @@ -430,13 +430,13 @@ def self.behavior 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/) + }.to raise_error(ArgumentError) end end describe "VALID_MODES" do it "contains save and compare" do - expect(DeprecationTracker::VALID_MODES).to contain_exactly(:save, :compare) + expect(DeprecationTracker::VALID_MODES).to include(:save, :compare) end it "accepts all valid modes without error" do