lint: fix spec/unit

- One lint (RSpec/SubjectStub) is disabled in two classes rather than being fixed,
  because fixing it really requires a rethink about the actual classes, and
  this set of commits is scoped to just making the lint pass. We should
  revisit how these two classes work later (especially the call out to mtime
  in ics_renderer)

spec/unit/api_error_routing_constraint_spec.rb

@@ -1,18 +1,19 @@
 RSpec.describe ApiErrorRoutingConstraint do
   include ContentStoreHelpers
 
-  let(:subject) { described_class.new }
-  let(:request) { double(path: "/slug", env: {}) }
+  subject(:api_error_routing_constraint) { described_class.new }
+
+  let(:request) { instance_double(ActionDispatch::Request, path: "/slug", env: {}) }
 
   it "returns true if there's a cached error" do
     stub_content_store_does_not_have_item("/slug")
 
-    expect(subject.matches?(request)).to be true
+    expect(api_error_routing_constraint.matches?(request)).to be true
   end
 
   it "returns false if there was no error in API calls" do
     stub_content_store_has_item("/slug")
 
-    expect(subject.matches?(request)).to be false
+    expect(api_error_routing_constraint.matches?(request)).to be false
   end
 end

spec/unit/calendar_spec.rb

@@ -5,10 +5,10 @@
     mock_calendar_fixtures
   end
 
-  context "finding a calendar by slug" do
+  context "when finding a calendar by slug" do
     it "constructs a calendar with the slug and data from the corresponding JSON file" do
       data_from_json = JSON.parse(File.read(Rails.root.join(Calendar::REPOSITORY_PATH, "single-calendar.json")))
-      expect(described_class).to receive(:new).with("single-calendar", data_from_json).and_return(:a_calendar)
+      allow(described_class).to receive(:new).with("single-calendar", data_from_json).and_return(:a_calendar)
       cal = described_class.find("single-calendar")
 
       expect(cal).to eq(:a_calendar)
@@ -28,8 +28,8 @@
   end
 
   describe "#divisions" do
-    before do
-      @cal = described_class.new(
+    subject(:calendar) do
+      described_class.new(
         "a-calendar",
         "title" => "UK bank holidays",
         "divisions" => {
@@ -41,114 +41,116 @@
     end
 
     it "constructs a division for each one in the data" do
-      expect(Calendar::Division).to receive(:new).with("england-and-wales", { "2012" => [1], "2013" => [3] }).and_return(:england_and_wales)
-      expect(Calendar::Division).to receive(:new).with("scotland", { "2012" => [1, 2], "2013" => [3, 4] }).and_return(:scotland)
-      expect(Calendar::Division).to receive(:new).with("northern-ireland", { "2012" => [2], "2013" => [4] }).and_return(:northern_ireland)
-      expect(@cal.divisions).to eq(%i[england_and_wales scotland northern_ireland])
+      allow(Calendar::Division).to receive(:new).with("england-and-wales", { "2012" => [1], "2013" => [3] }).and_return(:england_and_wales)
+      allow(Calendar::Division).to receive(:new).with("scotland", { "2012" => [1, 2], "2013" => [3, 4] }).and_return(:scotland)
+      allow(Calendar::Division).to receive(:new).with("northern-ireland", { "2012" => [2], "2013" => [4] }).and_return(:northern_ireland)
+
+      expect(calendar.divisions).to eq(%i[england_and_wales scotland northern_ireland])
     end
 
     it "caches the constructed instances" do
-      first = @cal.divisions
+      first = calendar.divisions
 
       expect(Calendar::Division).not_to receive(:new)
-      expect(@cal.divisions).to eq(first)
+      expect(calendar.divisions).to eq(first)
     end
 
-    context "finding a division by slug" do
+    context "when finding a division by slug" do
       it "returns the division with the matching slug" do
-        div = @cal.division("england-and-wales")
+        div = calendar.division("england-and-wales")
 
         expect(div.class).to eq(Calendar::Division)
         expect(div.title).to eq("England and wales")
       end
 
       it "raises exception when division doesn't exist" do
-        expect { @cal.division("non-existent") }.to raise_error(Calendar::CalendarNotFound)
+        expect { calendar.division("non-existent") }.to raise_error(Calendar::CalendarNotFound)
       end
     end
   end
 
   describe "#events" do
+    subject(:calendar) { described_class.new("a-calendar") }
+
+    let(:divisions) { [] }
+
     before do
-      @divisions = []
-      @calendar = described_class.new("a-calendar")
-      allow(@calendar).to receive(:divisions).and_return(@divisions)
+      allow(calendar).to receive(:divisions).and_return(divisions) # rubocop:disable RSpec/SubjectStub
     end
 
     it "merges events for all years into single array" do
-      @divisions << instance_double(Calendar::Division, events: [1, 2])
-      @divisions << instance_double(Calendar::Division, events: [3, 4, 5])
-      @divisions << instance_double(Calendar::Division, events: [6, 7])
+      divisions << instance_double(Calendar::Division, events: [1, 2])
+      divisions << instance_double(Calendar::Division, events: [3, 4, 5])
+      divisions << instance_double(Calendar::Division, events: [6, 7])
 
-      expect(@calendar.events).to eq([1, 2, 3, 4, 5, 6, 7])
+      expect(calendar.events).to eq([1, 2, 3, 4, 5, 6, 7])
     end
 
     it "handles years with no events" do
-      @divisions << instance_double(Calendar::Division, events: [1, 2])
-      @divisions << instance_double(Calendar::Division, events: [])
-      @divisions << instance_double(Calendar::Division, events: [6, 7])
+      divisions << instance_double(Calendar::Division, events: [1, 2])
+      divisions << instance_double(Calendar::Division, events: [])
+      divisions << instance_double(Calendar::Division, events: [6, 7])
 
-      expect(@calendar.events).to eq([1, 2, 6, 7])
+      expect(calendar.events).to eq([1, 2, 6, 7])
     end
   end
 
-  context "attribute accessors" do
-    before do
-      @cal = described_class.new("a-calendar", "title" => "bank_holidays.calendar.title", "description" => "bank_holidays.calendar.description")
-    end
+  describe "attribute accessors" do
+    subject(:calendar) { described_class.new("a-calendar", "title" => "bank_holidays.calendar.title", "description" => "bank_holidays.calendar.description") }
 
     it "has an accessor for the title" do
-      expect(@cal.title).to eq("UK bank holidays")
+      expect(calendar.title).to eq("UK bank holidays")
     end
 
     it "has an accessor for the description" do
-      expect(@cal.description).to eq("Find out when bank holidays are in England, Wales, Scotland and Northern Ireland - including past and future bank holidays")
+      expect(calendar.description).to eq("Find out when bank holidays are in England, Wales, Scotland and Northern Ireland - including past and future bank holidays")
     end
   end
 
   describe "#show_bunting?" do
-    before { @cal = described_class.new("a-calendar") }
+    subject(:calendar) { described_class.new("a-calendar") }
 
     it "is true when one division is buntable" do
-      @div1 = instance_double(Calendar::Division, show_bunting?: true)
-      @div2 = instance_double(Calendar::Division, show_bunting?: false)
-      @div3 = instance_double(Calendar::Division, show_bunting?: false)
-      allow(@cal).to receive(:divisions).and_return([@div1, @div2, @div3])
+      div1 = instance_double(Calendar::Division, show_bunting?: true)
+      div2 = instance_double(Calendar::Division, show_bunting?: false)
+      div3 = instance_double(Calendar::Division, show_bunting?: false)
+      allow(calendar).to receive(:divisions).and_return([div1, div2, div3]) # rubocop:disable RSpec/SubjectStub
 
-      expect(@cal.show_bunting?).to be true
+      expect(calendar.show_bunting?).to be true
     end
 
     it "is true when more than one division is buntable" do
-      @div1 = instance_double(Calendar::Division, show_bunting?: true)
-      @div2 = instance_double(Calendar::Division, show_bunting?: true)
-      @div3 = instance_double(Calendar::Division, show_bunting?: false)
-      allow(@cal).to receive(:divisions).and_return([@div1, @div2, @div3])
+      div1 = instance_double(Calendar::Division, show_bunting?: true)
+      div2 = instance_double(Calendar::Division, show_bunting?: true)
+      div3 = instance_double(Calendar::Division, show_bunting?: false)
+      allow(calendar).to receive(:divisions).and_return([div1, div2, div3]) # rubocop:disable RSpec/SubjectStub
 
-      expect(@cal.show_bunting?).to be true
+      expect(calendar.show_bunting?).to be true
     end
 
     it "is false when no divisions are buntable" do
-      @div1 = instance_double(Calendar::Division, show_bunting?: false)
-      @div2 = instance_double(Calendar::Division, show_bunting?: false)
-      @div3 = instance_double(Calendar::Division, show_bunting?: false)
-      allow(@cal).to receive(:divisions).and_return([@div1, @div2, @div3])
+      div1 = instance_double(Calendar::Division, show_bunting?: false)
+      div2 = instance_double(Calendar::Division, show_bunting?: false)
+      div3 = instance_double(Calendar::Division, show_bunting?: false)
+      allow(calendar).to receive(:divisions).and_return([div1, div2, div3]) # rubocop:disable RSpec/SubjectStub
 
-      expect(@cal.show_bunting?).to be false
+      expect(calendar.show_bunting?).to be false
     end
   end
 
   describe "#as_json" do
+    subject(:calendar) { described_class.new("a-calendar") }
+
     before do
-      @div1 = instance_double(Calendar::Division, slug: "division-1", as_json: "div1 json")
-      @div2 = instance_double(Calendar::Division, slug: "division-2", as_json: "div2 json")
-      @cal = described_class.new("a-calendar")
-      allow(@cal).to receive(:divisions).and_return([@div1, @div2])
+      div1 = instance_double(Calendar::Division, slug: "division-1", as_json: "div1 json")
+      div2 = instance_double(Calendar::Division, slug: "division-2", as_json: "div2 json")
+      allow(calendar).to receive(:divisions).and_return([div1, div2]) # rubocop:disable RSpec/SubjectStub
     end
 
     it "construct a hash representation of all divisions" do
       expected = { "division-1" => "div1 json", "division-2" => "div2 json" }
 
-      expect(@cal.as_json).to eq(expected)
+      expect(calendar.as_json).to eq(expected)
     end
   end
 end

spec/unit/content_item_loader_spec.rb

@@ -1,13 +1,14 @@
 RSpec.describe ContentItemLoader do
   include ContentStoreHelpers
 
-  let(:subject) { described_class.new }
+  subject(:content_item_loader) { described_class.new }
+
   let!(:item_request) { stub_content_store_has_item("/my-random-item") }
 
   describe ".for_request" do
     it "returns a new object per request" do
-      request_1 = double(path: "/my-random-item", env: {})
-      request_2 = double(path: "/my-random-item", env: {})
+      request_1 = instance_double(ActionDispatch::Request, path: "/my-random-item", env: {})
+      request_2 = instance_double(ActionDispatch::Request, path: "/my-random-item", env: {})
 
       loader_1 = described_class.for_request(request_1)
       loader_2 = described_class.for_request(request_2)
@@ -16,7 +17,7 @@
     end
 
     it "returns the same object for the same request" do
-      request_1 = double(path: "/my-random-item", env: {})
+      request_1 = instance_double(ActionDispatch::Request, path: "/my-random-item", env: {})
 
       loader_1 = described_class.for_request(request_1)
       loader_2 = described_class.for_request(request_1)
@@ -27,15 +28,15 @@
 
   describe "#load" do
     it "caches calls to the content store" do
-      subject.load("/my-random-item")
-      subject.load("/my-random-item")
+      content_item_loader.load("/my-random-item")
+      content_item_loader.load("/my-random-item")
 
       expect(item_request).to have_been_made.once
     end
 
     it "restricts cache to the specific instance of the class, so does not cache across requests" do
-      request_1 = double(path: "/my-random-item", env: {})
-      request_2 = double(path: "/my-random-item", env: {})
+      request_1 = instance_double(ActionDispatch::Request, path: "/my-random-item", env: {})
+      request_2 = instance_double(ActionDispatch::Request, path: "/my-random-item", env: {})
 
       loader_1 = described_class.for_request(request_1)
       loader_2 = described_class.for_request(request_2)
@@ -52,11 +53,11 @@
       end
 
       it "returns (but does not raise) the original exception" do
-        expect(subject.load("/my-missing-item")).to be_a(GdsApi::HTTPErrorResponse)
+        expect(content_item_loader.load("/my-missing-item")).to be_a(GdsApi::HTTPErrorResponse)
       end
     end
 
-    context "With ALLOW_LOCAL_CONTENT_ITEM_OVERRIDE=true" do
+    context "with ALLOW_LOCAL_CONTENT_ITEM_OVERRIDE=true" do
       before do
         ENV["ALLOW_LOCAL_CONTENT_ITEM_OVERRIDE"] = "true"
         stub_const("ContentItemLoader::LOCAL_ITEMS_PATH", "spec/fixtures/local-content-items")
@@ -70,7 +71,7 @@
         let!(:item_request) { stub_content_store_has_item("/my-json-item") }
 
         it "loads content from the JSON file instead of the content store" do
-          response = subject.load("/my-json-item")
+          response = content_item_loader.load("/my-json-item")
 
           expect(item_request).not_to have_been_made
           expect(ContentItemFactory.build(response).schema_name).to eq("json_page")
@@ -81,7 +82,7 @@
         let!(:item_request) { stub_content_store_has_item("/my-yaml-item") }
 
         it "loads content from the YAML file instead of the content store" do
-          response = subject.load("/my-yaml-item")
+          response = content_item_loader.load("/my-yaml-item")
 
           expect(item_request).not_to have_been_made
           expect(ContentItemFactory.build(response).schema_name).to eq("yaml_page")
@@ -92,7 +93,7 @@
         let!(:item_request) { stub_content_store_has_item("/my-remote-item") }
 
         it "returns to loading from the content store" do
-          subject.load("/my-remote-item")
+          content_item_loader.load("/my-remote-item")
 
           expect(item_request).to have_been_made.once
         end

spec/unit/format_routing_constraint_spec.rb

@@ -2,7 +2,7 @@
   include ContentStoreHelpers
 
   describe "#matches?" do
-    let(:request) { double(params: { slug: "slug" }, env: {}) }
+    let(:request) { instance_double(ActionDispatch::Request, params: { slug: "slug" }, env: {}) }
 
     context "when the content_store returns a document" do
       before do

spec/unit/full_path_format_routing_constraint_spec.rb

@@ -2,7 +2,7 @@
   include ContentStoreHelpers
 
   describe "#matches?" do
-    let(:request) { double(path: "/format/routing/test", env: {}) }
+    let(:request) { instance_double(ActionDispatch::Request, path: "/format/routing/test", env: {}) }
 
     context "when the content_store returns a document" do
       before do