Upgrade devtools to 0.1.1 and RSpec to 3.3.1

backus commented 9 years ago

I removed the outdated Gemfile.devtools and pointed the Gemfile to the newly released devtools gem
I updated RSpec to 3.1.1 and updated the specs accordingly. The syntax conversions for RSpec 3 were performed using Transpec. I've preserved the conversion summary below.
I fixed a few rubocop violations caught by the rubocop upgrade. I also disabled a few cops that did not fit the style used in this repo
I fixed a few specs to silence rspec warnings which are new to RSpec 3
I adjusted the travis build matrix

See the individual commits for more detail

This conversion is done by Transpec 3.1.1 with the following command: transpec --keep its,oneliner

37 conversions from: obj.stub(:message) to: allow(obj).to receive(:message)

20 conversions from: obj.should_receive(:message) to: expect(obj).to receive(:message)

16 conversions from: == expected to: eq(expected)

For more details: https://github.com/yujinakayama/transpec#supported-conversions

backus commented 9 years ago

Anima's dependency on >= 2.1 breaks the build. I can add an updated travis matrix

backus commented 9 years ago

Ok I'm not quite sure why Travis is still failing now. It looks like it is saying there is a conflict between rspec versions it wants to use but I don't know where that conflict would be coming from.

backus commented 9 years ago

Updating the bundler development dependency fixed the bundling issue. Now new rubocop cops are failing the build since they aren't configured. I'll try to update the rubocop settings accordingly.

Since I expect to run into a few more build issues I'll split up my opinionated changes (like modifying rubocop settings) in separate commits which I can squash down, modify, or throw out later.

backus commented 9 years ago

Alright there are still some JRuby build issues which I'll look into (at least for jruby-head). @dkubb would it be fine to drop everything below 2.1.7 from the travis matrix?

The MRI builds pass except for mutant which finds new mutations now that it is updated. I'm happy to try and tackle these mutations but it might make sense in the context of a separate PR.

backus commented 9 years ago

Some builds fail due to a neutral failure quoted below. I'm going to add this to ignore_subjects for now

--- Neutral failure --- Original code was inserted unmutated. And the test did NOT PASS. Your tests do not pass initially or you found a bug in mutant / unparser. Subject AST:

(def :define
  (args)
  (begin
    (send nil :desc
      (dstr
        (str "Measure docs in ")
        (begin
          (send
            (ivar :@config) :path))
        (str " with yardstick")))
    (block
      (send nil :task
        (ivar :@name))
      (args)
      (send nil :yardstick_measure))))

Unparsed Source:

def define
  desc("Measure docs in #{@config.path} with yardstick")
  task(@name) do
    yardstick_measure
  end
end

Test Result:

- 10 @ runtime: 0.189897065
  - rspec:64:./spec/integration/yardstick/rake/measurement_spec.rb:17/Yardstick::Rake::Measurement should write the report
  - rspec:204:./spec/unit/yardstick/rake/measurement/initialize_spec.rb:18/Yardstick::Rake::Measurement#initialize with custom arguments when valid options 
  - rspec:205:./spec/unit/yardstick/rake/measurement/initialize_spec.rb:20/Yardstick::Rake::Measurement#initialize with custom arguments when valid options creates rake task with given name
  - rspec:206:./spec/unit/yardstick/rake/measurement/initialize_spec.rb:25/Yardstick::Rake::Measurement#initialize with custom arguments when valid options calls yardstick_measure when rake task is executed
  - rspec:207:./spec/unit/yardstick/rake/measurement/initialize_spec.rb:31/Yardstick::Rake::Measurement#initialize with custom arguments when valid options should include the threshold in the task name
  - rspec:208:./spec/unit/yardstick/rake/measurement/initialize_spec.rb:42/Yardstick::Rake::Measurement#initialize when with default arguments 
  - rspec:209:./spec/unit/yardstick/rake/measurement/initialize_spec.rb:44/Yardstick::Rake::Measurement#initialize when with default arguments assigns yardstick_measure as the name
  - rspec:210:./spec/unit/yardstick/rake/measurement/initialize_spec.rb:57/Yardstick::Rake::Measurement#initialize when block provided 
  - rspec:211:./spec/unit/yardstick/rake/measurement/initialize_spec.rb:59/Yardstick::Rake::Measurement#initialize when block provided should yield to Config
  - rspec:212:./spec/unit/yardstick/rake/measurement/yardstick_measure_spec.rb:15/Yardstick::Rake::Measurement#yardstick_measure writes yardstick results

Test Output:
Randomized with seed 42917
F
Failures:
  1) Yardstick::Rake::Measurement should write the report
     Failure/Error: expect(output.read)
     Errno::ENOENT:
       No such file or directory @ rb_sysopen - measurements/report.txt
     # ./spec/integration/yardstick/rake/measurement_spec.rb:19:in `read'
     # ./spec/integration/yardstick/rake/measurement_spec.rb:19:in `read'
     # ./spec/integration/yardstick/rake/measurement_spec.rb:19:in `block (2 levels) in <top (required)>'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-rspec-0.8.2/lib/mutant/integration/rspec.rb:68:in `call'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/env.rb:64:in `block in run_mutation_tests'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/isolation.rb:48:in `call'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/isolation.rb:48:in `block (3 levels) in call'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/isolation.rb:45:in `open'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/isolation.rb:45:in `block (2 levels) in call'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/isolation.rb:44:in `fork'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/isolation.rb:44:in `block in call'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/isolation.rb:41:in `pipe'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/isolation.rb:41:in `call'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/env.rb:62:in `run_mutation_tests'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/env.rb:39:in `kill'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/parallel/worker.rb:64:in `call'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/parallel/worker.rb:64:in `handle_job'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/parallel/worker.rb:47:in `handle'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/parallel/worker.rb:31:in `run'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/parallel/worker.rb:15:in `run'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/parallel/master.rb:49:in `block (2 levels) in run'
     # /home/travis/.rvm/gems/ruby-2.2.0/gems/mutant-0.8.5/lib/mutant/actor/env.rb:16:in `block in spawn'
Finished in 0.15698 seconds (files took 2 minutes 46.2 seconds to load)
1 example, 1 failure

mbj commented 9 years ago

Some builds fail due to a neutral failure quoted below. I'm going to add this to ignore_subjects for now

JFYI: In the presence of neutrals: You cannot know the killed evils are killed for real. So the coverage in presence of neutrals cannot be trusted.

dkubb commented 9 years ago

would it be fine to drop everything below 2.1.7 from the travis matrix?

@backus sure. If someone wants it added back in they can create a PR with the fixes and travis additions. I don't use anything except ruby 2.2.3 in production now anyway.

backus commented 9 years ago

@dkubb This should be ready for review

mbj commented 9 years ago

@backus I did a code only pass over the diff and noticed 2 class of problems I'd like to see addressed. On the first instance I left a verbose description, on each instance a link to the description. The idea is that if you fix the instances, the "instance comment" will not be visible in the PR view anymore, as a confirmation you catched all instance of a certain class. I realize you did the transform with transproc, but still I think we should always go for the best possible expectations.

OT: Conceptually I think github should provide better review tooling.

Also I notice that this code base still uses 1.8 hashes, I think we should add a dedicated commit to use 1.9 style hashes where possible.

backus commented 9 years ago

Fixed the mentioned problems @mbj and also used rubocop to update the hash syntax. The instance comments were actually really helpful for knowing if I addressed everything. They were also helpful for getting to the issues quickly (using jq):

curl 'https://api.github.com/repos/dkubb/yardstick/pulls/27/comments' | jq 'map(.path) | .[]' -r | sort | uniq | xargs -L1 [editor]

ready for another pass assuming CI builds

dkubb commented 9 years ago

@backus I finished a pass over this code. I think once the flagged issues are resolved I'm ready to merge.

dkubb commented 9 years ago

@backus looks great. I will merge once everything is green.

dkubb commented 9 years ago

@backus thanks for your contribution!

backus commented 9 years ago

:smile:

dkubb / yardstick

Upgrade devtools to 0.1.1 and RSpec to 3.3.1 #27