From df4f3bba4f51cbc8eb17d330068b3877b1684e5b Mon Sep 17 00:00:00 2001 From: Jared Beck Date: Mon, 18 Sep 2017 23:56:07 -0400 Subject: [PATCH] Lint: Fix various offenses, mostly rspec - Fix Lint/RescueWithoutErrorClass - And I use the term "fix" loosely - Fix RSpec/PredicateMatcher - The failure message should actually be better this way too. - RSpec/LetBeforeExamples - RSpec/HookArgument - Fix RSpec/ExpectInHook in widget_spec.rb - Fix RSpec/MessageSpies - I like spies. I've never linted them before, but I like how they separate test setup from assertions. --- .rubocop_todo.yml | 19 +++++-------------- lib/paper_trail/record_trail.rb | 2 +- lib/paper_trail/version_concern.rb | 4 ++-- spec/models/gadget_spec.rb | 4 ++-- spec/models/version_spec.rb | 9 +++++---- spec/models/widget_spec.rb | 12 ++++++++---- spec/paper_trail/model_spec.rb | 6 +++--- spec/spec_helper.rb | 4 ++-- 8 files changed, 28 insertions(+), 32 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 0ff2d85d..2bbf1e22 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -3,9 +3,6 @@ require: rubocop-rspec # Remove these configuration records # one by one as the offenses are removed from the code base. -Lint/RescueWithoutErrorClass: - Enabled: false - Metrics/AbcSize: Max: 22 # Goal: 15 @@ -22,24 +19,21 @@ RSpec/BeforeAfterAll: Enabled: false RSpec/ExpectInHook: - Enabled: false + Exclude: + - spec/paper_trail/associations_spec.rb + - spec/models/version_spec.rb RSpec/FilePath: Enabled: false -RSpec/HookArgument: - Enabled: false - RSpec/InstanceVariable: Exclude: - spec/paper_trail/associations_spec.rb - spec/paper_trail/model_spec.rb -RSpec/LetBeforeExamples: - Enabled: false - RSpec/MessageSpies: - Enabled: false + Exclude: + - spec/models/version_spec.rb RSpec/NamedSubject: Enabled: false @@ -49,8 +43,5 @@ RSpec/NestedGroups: - spec/paper_trail/associations_spec.rb - spec/paper_trail/model_spec.rb -RSpec/PredicateMatcher: - Enabled: false - Security/YAMLLoad: Enabled: false diff --git a/lib/paper_trail/record_trail.rb b/lib/paper_trail/record_trail.rb index 0d906fdb..d37ecb35 100644 --- a/lib/paper_trail/record_trail.rb +++ b/lib/paper_trail/record_trail.rb @@ -170,7 +170,7 @@ module PaperTrail def next_version subsequent_version = source_version.next subsequent_version ? subsequent_version.reify : @record.class.find(@record.id) - rescue # TODO: Rescue something more specific + rescue StandardError # TODO: Rescue something more specific nil end diff --git a/lib/paper_trail/version_concern.rb b/lib/paper_trail/version_concern.rb index 82b5f441..3fe66917 100644 --- a/lib/paper_trail/version_concern.rb +++ b/lib/paper_trail/version_concern.rb @@ -162,7 +162,7 @@ module PaperTrail def primary_key_is_int? @primary_key_is_int ||= columns_hash[primary_key].type == :integer - rescue + rescue StandardError # TODO: Rescue something more specific true end @@ -307,7 +307,7 @@ module PaperTrail else begin PaperTrail.serializer.load(object_changes) - rescue # TODO: Rescue something specific + rescue StandardError # TODO: Rescue something more specific {} end end diff --git a/spec/models/gadget_spec.rb b/spec/models/gadget_spec.rb index 255716ba..0889e61e 100644 --- a/spec/models/gadget_spec.rb +++ b/spec/models/gadget_spec.rb @@ -1,10 +1,10 @@ require "spec_helper" RSpec.describe Gadget, type: :model do - it { is_expected.to be_versioned } - let(:gadget) { Gadget.create!(name: "Wrench", brand: "Acme") } + it { is_expected.to be_versioned } + describe "updates", versioning: true do it "generates a version for updates to `name` attribute" do expect { gadget.update_attribute(:name, "Hammer") }.to(change { gadget.versions.size }.by(1)) diff --git a/spec/models/version_spec.rb b/spec/models/version_spec.rb index f896c3e0..3621be4e 100644 --- a/spec/models/version_spec.rb +++ b/spec/models/version_spec.rb @@ -65,15 +65,16 @@ module PaperTrail it "sets the invoke `paper_trail_originator`" do allow(ActiveSupport::Deprecation).to receive(:warn) subject = PaperTrail::Version.new - expect(subject).to receive(:paper_trail_originator) + allow(subject).to receive(:paper_trail_originator) subject.originator + expect(subject).to have_received(:paper_trail_originator) end it "displays a deprecation warning" do - expect(ActiveSupport::Deprecation).to receive(:warn). + allow(ActiveSupport::Deprecation).to receive(:warn) + PaperTrail::Version.new.originator + expect(ActiveSupport::Deprecation).to have_received(:warn). with(/Use paper_trail_originator instead of originator/) - subject = PaperTrail::Version.new - subject.originator end end diff --git a/spec/models/widget_spec.rb b/spec/models/widget_spec.rb index 5dc15501..ca1481db 100644 --- a/spec/models/widget_spec.rb +++ b/spec/models/widget_spec.rb @@ -1,12 +1,12 @@ require "spec_helper" RSpec.describe Widget, type: :model do + let(:widget) { Widget.create! name: "Bob", an_integer: 1 } + describe "`be_versioned` matcher" do it { is_expected.to be_versioned } end - let(:widget) { Widget.create! name: "Bob", an_integer: 1 } - describe "`have_a_version_with` matcher", versioning: true do before do widget.update_attributes!(name: "Leonard", an_integer: 1) @@ -145,8 +145,9 @@ RSpec.describe Widget, type: :model do it "does not clobber the IdentityMap when reifying" do widget.update_attributes name: "Henry", created_at: Time.now - 1.day widget.update_attributes name: "Harry" - expect(ActiveRecord::IdentityMap).to receive(:without).once + allow(ActiveRecord::IdentityMap).to receive(:without) widget.versions.last.reify + expect(ActiveRecord::IdentityMap).to have_receive(:without).once end end end @@ -249,10 +250,11 @@ RSpec.describe Widget, type: :model do before do PaperTrail.whodunnit = orig_name - expect(widget.versions.last.whodunnit).to eq(orig_name) # persist `widget` + widget # persist `widget` (call the `let`) end it "modifies value of `PaperTrail.whodunnit` while executing the block" do + expect(widget.versions.last.whodunnit).to eq(orig_name) widget.paper_trail.whodunnit(new_name) do expect(PaperTrail.whodunnit).to eq(new_name) widget.update_attributes(name: "Elizabeth") @@ -261,6 +263,7 @@ RSpec.describe Widget, type: :model do end it "reverts value of whodunnit to previous value after executing the block" do + expect(widget.versions.last.whodunnit).to eq(orig_name) widget.paper_trail.whodunnit(new_name) { |w| w.update_attributes(name: "Elizabeth") } @@ -268,6 +271,7 @@ RSpec.describe Widget, type: :model do end it "reverts to previous value, even if error within block" do + expect(widget.versions.last.whodunnit).to eq(orig_name) expect { widget.paper_trail.whodunnit(new_name) { raise } }.to raise_error(RuntimeError) diff --git a/spec/paper_trail/model_spec.rb b/spec/paper_trail/model_spec.rb index 8f083e97..be8cadc8 100644 --- a/spec/paper_trail/model_spec.rb +++ b/spec/paper_trail/model_spec.rb @@ -84,9 +84,9 @@ RSpec.describe(::PaperTrail, versioning: true) do end it "have versions that are not live" do - expect( - @widget.versions.map(&:reify).compact.all? { |w| !w.paper_trail.live? } - ).to(be_truthy) + @widget.versions.map(&:reify).compact.each do |v| + expect(v.paper_trail).not_to be_live + end end it "have stored changes" do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 5ce9dcf5..821eb9f1 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -72,7 +72,7 @@ RSpec.configure do |config| # Truncation is about three times slower than transaction rollback, so it'll # be nice when we can drop support for rails < 5. if active_record_gem_version < ::Gem::Version.new("5") - config.before(:each) { DatabaseCleaner.start } - config.after(:each) { DatabaseCleaner.clean } + config.before { DatabaseCleaner.start } + config.after { DatabaseCleaner.clean } end end