Rspec rubocop #4524

KludgeKML · 2024-12-11T16:32:14Z

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

What

During the rspec conversion, we missed that rubocop-govuk has an opt-in rspec lint ruleset, which wasn't turned on (because previously there was no rspec). This PR turns on that setting, and fixes the many cops that were triggered.

Why

We should adhere to our standards, especially as Frontend is the candidate for merging the other frontend apps.

- This is the "include dangerous autocorrects" version, and indeed some of these changes break tests. I will fix in the following commits.

- will be used in the following commits, allows us to include expectations in methods that would otherwise appear erroneously to have no expectations, fooling rubocop - But is also a more consistent call.

- 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)

leenagupte

This is looking good. I've added some inline comments, but they are quite small.

leenagupte · 2024-12-17T10:58:36Z

spec/presenter/get_involved_spec.rb

@@ -1,5 +1,5 @@
 RSpec.describe GetInvolved do
-  let(:consultation_1) do
+  let(:consultation_closing_november) do


These are much nicer names!

leenagupte · 2024-12-17T11:06:01Z

spec/requests/places_spec.rb

      end
    end

    context "with invalid postcode" do
      it "shows location error" do
        post "/slug", params: { postcode: invalid_postcode }

-        expect(LocationError.new(PlacesManagerResponse::INVALID_POSTCODE).postcode_error).to eq(@controller.view_assigns["location_error"].postcode_error)
+        expect(LocationError.new(PlacesManagerResponse::INVALID_POSTCODE).postcode_error).to eq(describecontroller.view_assigns["location_error"].postcode_error)


Typo? Should this be controller rather than describecontroller?

leenagupte · 2024-12-17T11:09:37Z

spec/requests/sessions_spec.rb

@@ -37,29 +38,31 @@
      end

      context "with a protocol-relative redirect" do
+        let(:referer) { "//protocol-relative-path" }
+        let(:redirect_path) { referer }


I don't think redirect_path is needed for this test if it's just a copy of referrer. However, I think that might be out of scope for these changes.

leenagupte · 2024-12-17T11:13:44Z

spec/requests/sessions_spec.rb

@@ -128,11 +131,11 @@
          get "/sign-in/callback", params: { code: "code123", state: "state123", _ga: underscore_ga }

          expect(response).to have_http_status(:redirect)
-          assert_includes(@response.redirect_url, underscore_ga)
+          assert_includes(response.redirect_url, underscore_ga)


Isn't assert_includes Minitest?
Should it be expect(response.redirect_url).to include(underscore_ga)?

leenagupte · 2024-12-17T11:14:40Z

spec/requests/sessions_spec.rb

@@ -142,10 +145,10 @@
          get "/sign-in/callback", params: { code: "code123", state: "state123" }

          expect(response).to have_http_status(:redirect)
-          assert_includes(@response.redirect_url, @redirect_path)
+          assert_includes(response.redirect_url, @redirect_path)


Same here and below about using assert_includes

leenagupte · 2024-12-17T13:42:49Z

spec/unit/services/csv_preview_service_spec.rb

      end
    end

    context "with CSV containing CRLF line terminators" do
-      subject { described_class.new(@crlf_csv).csv_rows }
+      let(:csv) { "header 1,header 2,header 3\ncontent 1,content 2,content 3\n".gsub(/\n/, "\r\n") }


Why not just set this to "header 1,header 2,header 3\ncontent 1,content 2,content 3\r\n?

leenagupte · 2024-12-17T13:48:28Z

spec/unit/simple_smart_answers/flow_spec.rb

+          "kind" => "outcome",
+          "slug" => "outcome-1",
+          "title" => "Outcome 1",
+          "body" => "<p>This is outcome 1</p>",


It's strange that Outcome 1 is duplicated. Perhaps it can be deleted?

leenagupte · 2024-12-17T13:58:12Z

spec/unit/sanitiser/strategy_spec.rb

So has this file has been renamed from "spec/unit/strategy_spec.rb"? If so, I'd expect to see a corresponding deleted file.

leenagupte · 2024-12-17T14:09:28Z

spec/unit/locales_validation_spec.rb

-  # Stop the checker putting it's jaunty message to stdout
-  around do |ex|
-    original_stdout = $stdout
-    $stdout = File.open(File::NULL, "w")
-    ex.run
-    $stdout = original_stdout
-  end
-


Thanks for removing this! I too want to see the error if the test fails.

leenagupte · 2024-12-17T14:10:28Z

spec/unit/strategy_spec.rb

Ah! This deletion should be fixed up to the previous commit.

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 11, 2024 16:32 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 11, 2024 17:09 Inactive

KludgeKML force-pushed the rspec-rubocop branch from 7e96a5e to 4604572 Compare December 12, 2024 10:11

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 12, 2024 10:12 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 12, 2024 13:53 Inactive

KludgeKML force-pushed the rspec-rubocop branch from ac9e058 to 8fab2f4 Compare December 12, 2024 16:12

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 12, 2024 16:13 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 12, 2024 16:29 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 12, 2024 17:19 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 12, 2024 17:27 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 16, 2024 11:49 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 16, 2024 11:59 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 16, 2024 14:19 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 16, 2024 15:01 Inactive

KludgeKML force-pushed the rspec-rubocop branch from c71e2b1 to ca01601 Compare December 16, 2024 15:13

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 16, 2024 15:13 Inactive

KludgeKML force-pushed the rspec-rubocop branch from ca01601 to ca83948 Compare December 16, 2024 15:18

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 16, 2024 15:18 Inactive

KludgeKML added 8 commits December 16, 2024 16:22

configure rubocop for rspec

9238dbf

run rubocop --autocorrect

cd6324a

Run rubocop -A

6dab178

- This is the "include dangerous autocorrects" version, and indeed some of these changes break tests. I will fix in the following commits.

Fix tests broken by rubocop -A

54ad64c

Fix lints in spec/components

e8c538d

lint: fix spec/helpers

d28abf6

lint: fix spec/models

e4517a5

lint: fix spec/presenters

b64750f

KludgeKML force-pushed the rspec-rubocop branch from ca83948 to 0f69853 Compare December 16, 2024 16:26

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 16, 2024 16:26 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 16, 2024 17:04 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 16, 2024 19:54 Inactive

KludgeKML force-pushed the rspec-rubocop branch from cc90ef8 to 20e057c Compare December 16, 2024 19:57

govuk-ci temporarily deployed to govuk-frontend-app-pr-4524 December 16, 2024 19:57 Inactive

KludgeKML added 12 commits December 16, 2024 19:59

Convert method to matcher

ef97c27

- will be used in the following commits, allows us to include expectations in methods that would otherwise appear erroneously to have no expectations, fooling rubocop - But is also a more consistent call.

lint: fix spec/requests

c748cf7

lint: fix spec/requests added

bf30659

lint: fix spec/routing

fb14dd6

lint: fix spec/support

5cace6c

lint: fix spec/system

25ad2a9

lint: fix spec/unit/presenters

e4a8bf8

lint: fix spec/unit/services

d5a1904

lint: fix spec/unit/simple_smart_answers

7f7901d

lint: fix spec/unit/calendar

13895bb

lint: fix spec/unit/sanitiser

dd5e5d0

KludgeKML force-pushed the rspec-rubocop branch from 20e057c to eee8251 Compare December 16, 2024 19:59

govuk-ci deployed to govuk-frontend-app-pr-4524 December 16, 2024 20:00 View deployment

KludgeKML requested a review from leenagupte December 16, 2024 20:01

KludgeKML marked this pull request as ready for review December 17, 2024 09:37

leenagupte reviewed Dec 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rspec rubocop #4524

Rspec rubocop #4524

KludgeKML commented Dec 11, 2024 •

edited

Loading

leenagupte left a comment

leenagupte Dec 17, 2024

leenagupte Dec 17, 2024

leenagupte Dec 17, 2024

leenagupte Dec 17, 2024

leenagupte Dec 17, 2024

leenagupte Dec 17, 2024

leenagupte Dec 17, 2024

leenagupte Dec 17, 2024

leenagupte Dec 17, 2024

leenagupte Dec 17, 2024

Rspec rubocop #4524

Are you sure you want to change the base?

Rspec rubocop #4524

Conversation

KludgeKML commented Dec 11, 2024 • edited Loading

What

Why

leenagupte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KludgeKML commented Dec 11, 2024 •

edited

Loading