JEG2 / highline

A higher level command-line oriented interface.
Other
1.29k stars 137 forks source link

can't modify frozen NilClass when using HighLine::Simulate #209

Closed ngw closed 7 years ago

ngw commented 7 years ago

Hello, I'm trying to test a Thor app that is using HighLine to handle I/O. The bit of code I'm attempting to validate is:

Cobain::Settings::SETTINGS.each do |setting|
  if Cobain::Settings.send(setting).nil?
    Cobain::Settings.send("#{setting}=", highline.ask(Cobain::Settings::OPTIONS[setting]))
  end
end

where highline is obviously a HighLine instance. This is my test:

def test_foo
  HighLine::Simulate.with("foo", "bar") do
    Cobain::CLI.new.invoke(:generate, [File.join(File.dirname(__FILE__), "fixtures", "nonexistant.yml")])
  end
end

Given that Cobain::Settings is an array of 2, it should call highline.ask twice (and it does). My understanding of HighLine::Simulate is that it should answer with "foo" at the first .ask call, and with "bar" at the second one. Unfortunately it fails with:

1) Error: CobainCliTest#test_when_settings_file_doesnt_exist_warns_about_generating_new_one_and_asks_missing_data:
RuntimeError: can't modify frozen NilClass
    /Users/ngw/.rvm/gems/ruby-2.4.0@cobain/gems/highline-1.7.8/lib/highline/simulate.rb:45:in `instance_variable_set'
    /Users/ngw/.rvm/gems/ruby-2.4.0@cobain/gems/highline-1.7.8/lib/highline/simulate.rb:45:in `ensure in with'
    /Users/ngw/.rvm/gems/ruby-2.4.0@cobain/gems/highline-1.7.8/lib/highline/simulate.rb:45:in `with'
    /Users/ngw/cobain/test/cli_test.rb:6:in `test_when_settings_file_doesnt_exist_warns_about_generating_new_one_and_asks_missing_data'

This doesn't seem to be new, as in #142, and honestly after looking at the code I have no idea what's happening, and where this nil comes from: ask should receive "foo" and "bar". Can someone lend me a hand? Much appreciated

abinoam commented 7 years ago

Dear @ngw,

Thank you for reporting the issue, we really appreciate it.

You're using version 1.7.8. Line 45 of lib/highline/simulate.rb is:

 $terminal.instance_variable_set :@input, @input

The message error is RuntimeError: can't modify frozen NilClass

I guess $terminal global variable is not set when instance_variable_set is called (for the context you're giving). So, if $terminal is nil then calling instance_variable_set on it will trigger this can't modify frozen NilClass error message.

But, the fragment highline.ask(Cobain::Settings::OPTIONS[setting]) may also be problematic as highline should be equal to $terminal for HighLine::Simulate.with to work well.

I was able to set a small snippet to show what I think could be the cause.

require 'highline/simulate'
HighLine::Simulate.with("foo", "bar") { 2.times { puts ask "Say it again" } }
# => RuntimeError: can't modify frozen NilClass

# Now we require 'highline/import' that sets $terminal to a HighLine instance.
# The same line now will work as expected
require 'highline/import'
HighLine::Simulate.with("foo", "bar") { 2.times { puts ask "Say it again" } }
# => Work well

# And it works the same if using `$terminal.ask`
HighLine::Simulate.with("foo", "bar") { 2.times { puts $terminal.ask "Say it again" } }
# => Work well

# But if I create my own HighLine instance, it won't work as expected.
highline = HighLine.new
HighLine::Simulate.with("foo", "bar") { 2.times { puts highline.ask "Say it again" } }
# => It fails

So, my suggestions are:

I may agree that this behaviour is weird. We are slowling deprecating this global variable thing. You can se by the docs that we favor using cli = HighLine.new and cli.ask flow for the 2.0.0-pre versions. But, we haven't refactored the HighLine::Simulate class. I'll put this on my priority list.

Please, give us some feedback if you could make your tests pass with the above informations. And feel free to ask any futher question.

ngw commented 7 years ago

Unfortunately Thor already has an .ask method: https://github.com/erikhuda/thor/blob/master/lib/thor/shell/basic.rb#L72-L81 Tomorrow I'll try your suggestions and report here what I find out, thanks a lot.

abinoam commented 7 years ago

Name collisions like this made us rethink the way we have been messing with global namespace. Unfortunately we haven't made all changes we would like to make.

I think there's a better workaround then.

require 'highline'
require 'highline/simulate'
simulator = HighLine::Simulate.new(["foo", "bar"])
cli = HighLine.new(simulator) # Sets HighLine input to the simulator instance
2.times { puts cli.ask "Asking something to the simulator" }
ngw commented 7 years ago

It actually worked, I only had to mock HighLine.new to return the new simulator instance. Some docs about testing would really be useful I guess.

abinoam commented 7 years ago

I'll close the issue. Feel free to reopen it if you want to add any information. Thanks for contributing opening the issue and discussing it.