formulary_spec: update API tests to avoid mocking #16697
Conversation
Stop mocking the formulary loader method. We just need to set the environment variable so that it knows to load things from the API. Fix spec that doesn't work with the `CoreTap.formula_names`. That method assumes that we always have a valid `ruby_source_path` in the API JSON even though that was optional for a while after launching the API if my memory serves me. It's probably fine to assume this should always be set though and I changed it to use `Hash#fetch` to give use better error messages if something goes wrong in the future. Remove unused `allow(x).to receive(y).and_call_original` in tap spec.
| new_path = File.join(tap_path, formula_hash["ruby_source_path"]) # Pathname equivalent is slow in a tight loop | ||
| # Pathname equivalent is slow in a tight loop | ||
| new_path = File.join(tap_path, formula_hash.fetch("ruby_source_path")) |
There was a problem hiding this comment.
File#join expects one or more strings while Array#join handles nil arguments gracefully.
irb(main):001:0> ["one", nil].join
=> "one"
irb(main):002:0> File.join("one", nil)
(irb):2:in `join': no implicit conversion of nil into String (TypeError)
from (irb):2:in `<main>'
from /Users/kevinrobell/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/irb-1.6.2/exe/irb:11:in `<top (required)>'
from /Users/kevinrobell/.asdf/installs/ruby/3.2.2/bin/irb:25:in `load'
from /Users/kevinrobell/.asdf/installs/ruby/3.2.2/bin/irb:25:in `<main>'
There was a problem hiding this comment.
As long as the API JSON is current, "ruby_source_path" will always be populated, right?
There was a problem hiding this comment.
In any case this doesn't change the behaviour in case of nil, only the error message.
There was a problem hiding this comment.
As long as the API JSON is current,
"ruby_source_path"will always be populated, right?
Correct.
In any case this doesn't change the behaviour in case of
nil, only the error message.
Yep, and it also better shows intent.
Yeah was likely the cause of https://github.com/orgs/Homebrew/discussions/4701 and https://github.com/orgs/Homebrew/discussions/4750, but it's been like that for a while now |
|
If we wanted to be really cautious, we could just skip any values that don't have this anymore but I assume the time to do that has passed. |
|
The fix would be more defaulting to something like |
|
Thanks @apainintheneck! |
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?Stop mocking the formulary loader method. We just need to set the environment variable so that it knows to load things from the API.
Fix spec that doesn't work with the
CoreTap#formula_names. That method assumes that we always have a validruby_source_pathin the API JSON even though that was optional for a while after launching the API if my memory serves me. It's probably fine to assume this should always be set though and I changed it to useHash#fetchto give use better error messages if something goes wrong in the future.Remove unnecessary
allow(x).to receive(y).and_call_originalin tap spec.Extracted from #16460 (comment)