jejacks0n / teaspoon

Teaspoon: Javascript test runner for Rails. Use Selenium, BrowserStack, or PhantomJS.
1.43k stars 243 forks source link

Deprecates suite.use_framework= #394

Closed sl4m closed 9 years ago

sl4m commented 9 years ago

This bit me today and I yak shaved for a couple of hours. In my config, I had the following:

config.suite do |suite|
  suite.use_framework = :jasmine, "1.3.1"
end

Teaspoon accepted the configuration except every time I ran the rake task, it would raise an exception:

Teaspoon::UnknownFramework: Teaspoon::UnknownFramework
/home/v/.gem/ruby/2.2.2/gems/teaspoon-1.0.2/lib/teaspoon/registry.rb:24:in `fetch'
/home/v/.gem/ruby/2.2.2/gems/teaspoon-1.0.2/lib/teaspoon/configuration.rb:88:in `use_framework'
/home/v/code/spec/javascripts/config.rb:6:in `block (2 levels) in <top (required)>'
/home/v/.gem/ruby/2.2.2/gems/teaspoon-1.0.2/lib/teaspoon/configuration.rb:82:in `initialize'
/home/v/.gem/ruby/2.2.2/gems/teaspoon-1.0.2/lib/teaspoon/configuration.rb:59:in `new'
/home/v/.gem/ruby/2.2.2/gems/teaspoon-1.0.2/lib/teaspoon/configuration.rb:59:in `suite'
/home/v/code/spec/javascripts/config.rb:5:in `block in <top (required)>'
/home/v/.gem/ruby/2.2.2/gems/teaspoon-1.0.2/lib/teaspoon/configuration.rb:167:in `configure'
/home/v/code/spec/javascripts/config.rb:1:in `<top (required)>'

A bit of debugging revealed this:

[1] pry(main)> class Test
[1] pry(main)*   def use(one, two = nil)
[1] pry(main)*     puts "one #{one}"
[1] pry(main)*     puts "two #{two}"
[1] pry(main)*   end
[1] pry(main)*   alias_method :use=, :use
[1] pry(main)* end
=> Test
[2] pry(main)> test = Test.new
=> #<Test:0x007f92b6ee8ea0>
[3] pry(main)> test.use = :jasmine, '1.3.1'
one [:jasmine, "1.3.1"]
two
=> [:jasmine, "1.3.1"]
[4] pry(main)> test.use = :jasmine
one jasmine
two
=> :jasmine
[5] pry(main)> test.use(:jasmine, '1.3.1')
one jasmine
two 1.3.1
=> nil

The only time the use_framework= method works is when you pass in a single argument (framework name). Is this method worth keeping around?

mikepack commented 9 years ago

Good catch, and sorry about the yak shaving. Programming, right?

I don't think it would be wise to straight remove the alias, as this would introduce a breaking change. We should define use_framework=, provide the correct functionality, and deprecate the method.

sl4m commented 9 years ago

Good point. I can work on explicitly defining use_framework= to handle my case and deprecate the method.

mikepack commented 9 years ago

Thanks. We try to isolate deprecations here via monkey patching. It seems like you could remove the alias, and define the method there.

sl4m commented 9 years ago

Sounds good. Shall I close this request and open a new one when ready?

mikepack commented 9 years ago

Seems reasonable to keep this open for now and just push changes to this PR.

sl4m commented 9 years ago

I think aruba 0.8.x introduced some breaking changes that is causing the CI builds to fail.

mikepack commented 9 years ago

Damn, that Aruba upgrade was painful.

Thanks so much for this high quality PR :heart:

jejacks0n commented 9 years ago

:+1:

sl4m commented 9 years ago

Thanks for merging! :+1: