Add VALID_MODES constant and validate tracker mode at initialization#186
Add VALID_MODES constant and validate tracker mode at initialization#186JuanVqz wants to merge 2 commits into
Conversation
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.
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.
etagwerker
left a comment
etagwerker
left a comment
There was a problem hiding this comment.
@JuanVqz I think this might introduced an unwanted side effect (?)
bin/deprecations run without --tracker-mode is probably broken. The CLI builds:
command = "DEPRECATION_TRACKER=#{tracker_mode} #{rspec_command} ..."
When tracker_mode is nil, the spawned child sees ENV["DEPRECATION_TRACKER"] = ""
Empty string is truthy in Ruby, so:
mode = opts[:mode] || ENV["DEPRECATION_TRACKER"] || :save # => ""
@mode = mode ? mode.to_sym : :save # => :""
# new check raises ArgumentError
Before this PR that silently no-op'd in after_run. After this PR it crashes at construction.
| it "raises ArgumentError for invalid mode" do | ||
| expect { | ||
| DeprecationTracker.new(shitlist_path, nil, "random_stuff") | ||
| }.to raise_error(ArgumentError) |
There was a problem hiding this comment.
is this change intentional? because it's not testing the same thing as before
now it's testing that "at some point" it raises an error, but it's not really testing that it's not calling save or compare, the code could change to call save and fail with an ArgumentError caused by some code after that and this test would still pass, even if the failure is not caused by the invalid mode
now it's testing an implementation detail when before it was testing the actual expected behavior of not creating a sideeffect calling those methods
I think the previous test was better
| stub_const("ENV", ENV.to_h.merge("DEPRECATION_TRACKER" => "bogus")) | ||
| expect { | ||
| DeprecationTracker.init_tracker({}) | ||
| }.to raise_error(ArgumentError) |
There was a problem hiding this comment.
you should probably include the error message or at least part of the exception message in the expectation
because now ANY ArgumentError thrown would satisfy this test, even if unrelated to a wrong tracker mode, it's not exactly testing that the reason it failed was the invalid value
3 participants
Description
Introduces
DeprecationTracker::VALID_MODESas a single source of truth for accepted tracker modes (save,compare). Constants live inlib/deprecation_tracker/valid_modes.rbso theexe/deprecationsCLI can load them without pulling in the full library.Motivation and Context
Valid mode values were duplicated as inline literals across
lib/deprecation_tracker.rbandexe/deprecations. Adding or removing a mode required updating every literal. WithVALID_MODES, one change propagates to all validation, error messages, and CLI help text automatically.Additionally, passing an invalid mode previously silently no-oped (neither
savenorcompareran after the test suite). It now raisesArgumentErrorearly, covering both the library and CLI entry points.How Has This Been Tested?
ArgumentErrorENV['DEPRECATION_TRACKER']value viainit_trackerVALID_MODESdescribe block: constant membership and all valid modes construct without errorbundle exec rspec spec/deprecation_tracker_spec.rb)Screenshots:
N/A
I will abide by the code of conduct