Integrate Rubocop Rspec and Resolve Issues Raised by Rubocop Rspec #51

toshitapandey · 2022-10-08T07:12:13Z

Solves

Purpose

Install Rubocop Rspec
Configure Rubocop Rspec
Resolve issues raised by Rubocop Rspec

toshitapandey · 2022-10-08T07:17:29Z

@rodrigoapereira - There were 164 issues raised by rubocop-rspec. There are 54 pending issues, which I am not sure how to resolve. Please review.

rodrigoapereira · 2022-10-10T12:59:37Z

@toshitapandey Thank you! We appreciate your contribution, and I will review asap.

rodrigoapereira

I made some suggestions, could you fix? Also I will invite another maintainer to review your PR as well.

rodrigoapereira · 2022-10-10T15:09:36Z

spec/cassette/authentication/filter_spec.rb

@@ -22,7 +22,7 @@

      it_behaves_like 'with NOAUTH' do
        it 'never checks the role' do
-          expect(current_user).not_to receive(:has_raw_role?)
+          allow(current_user).not_to receive(:has_raw_role?)


I'd received an error when I ran rspec tests, because the use of not_to here. I'd recommend remove this allow, because if the method be called it will raise an exception, and made the test fail with:

#<InstanceDouble(Cassette::Authentication::User) (anonymous)> received unexpected message :has_raw_role? with (:something)

The spec output was:

Cassette::Authentication::Filter when controller belongs to Rails 3 behaves like controller behavior #validate_raw_role! behaves like with NOAUTH never checks the role Failure/Error: allow(current_user).not_to receive(:has_raw_role?) `allow(...).not_to receive` is not supported since it doesn't really make sense. What would it even mean?

Hum... I'm wondering here if this call doesn't make a network call underneath. Maybe we can just safely move to a expect(...).to have_received instead.

Leaving this test as is maybe lead us to false-positives when running rspec.
I wrote down a more complete suggestion in another part of the changes, @toshitapandey - can you see if it applies?

rodrigoapereira · 2022-10-10T15:10:48Z

spec/cassette/authentication/filter_spec.rb

@@ -56,7 +56,7 @@

      it_behaves_like 'with NOAUTH' do
        it 'never checks the role' do
-          expect(current_user).not_to receive(:has_role?)
+          allow(current_user).not_to receive(:has_role?)


The same issue of previous comment, could you fix this?

rodrigoapereira · 2022-10-10T17:13:37Z

spec/integration/cas/client_spec.rb

-    #   end
-    # end
-  end
+      it do


Could you merge these two its, naming as

it 'returns the configured username' do

Maybe this won't be possible due to rubocop - it may be checking for "one expect per it" rule. But I'm favorable for adding a description for each it. What about

it "returns an user object` do #... end it "instantiates the returned user with the correct login information" do #... end

spec/cassette/authentication/cache_spec.rb

rhruiz · 2022-10-10T17:45:57Z

spec/cassette/authentication/filter_spec.rb

@@ -34,20 +34,20 @@
      it 'forwards to current_user' do
        role = instance_double(String)

-        expect(current_user).to receive(:has_raw_role?).with(role).and_return(true)
+        allow(current_user).to receive(:has_raw_role?).with(role).and_return(true)


the spec says the method is supposed to call has_raw_role, so expecting that would make sense. If it is rubocop that is suggesting this change, you can add an expect(current_user).to have_received(:has_raw_role?).with(role) after the exercise

We can not add expect(current_user).to have_received(:has_raw_role?).with(role) as it is throwing following error:

The allow directive is used to create spies in the called method, allowing you to expect (or not) that it was called. I'm not sure about the rspec internal implementation of this method in specific, but, in summary, whenever you want to expect(object).to have received(:something), you must ensure that this object have already an spy on it listening to calls for #something method. In other words, you need a allow(object).to receive(:something) before the expectation.

rhruiz · 2022-10-10T17:51:07Z

spec/cassette/authentication/user_spec.rb

@@ -8,63 +8,93 @@
  describe '#initialize' do
    context 'without a config' do
      it 'forwards authorities parsing' do
-        expect(Cassette::Authentication::Authorities).to receive(:new).with('[CUSTOMERAPI, SAPI]', nil)
-        Cassette::Authentication::User.new(login: 'john.doe', name: 'John Doe', authorities: '[CUSTOMERAPI, SAPI]')
+        allow(Cassette::Authentication::Authorities).to receive(:new).with('[CUSTOMERAPI, SAPI]', nil)


Again it is expected to call the method, so a expect(...).to have_received is advised after the exercise

rhruiz · 2022-10-10T17:51:32Z

spec/cassette/authentication/user_spec.rb

-        Cassette::Authentication::User.new(login: 'john.doe', name: 'John Doe',
-                                           authorities: '[CUSTOMERAPI, SAPI]', config: config)
+        described_class.new(login: 'john.doe', name: 'John Doe',
+                            authorities: '[CUSTOMERAPI, SAPI]', config: config)


another method forwarding

rhruiz · 2022-10-10T17:55:31Z

spec/cassette/authentication_spec.rb

@@ -74,23 +80,27 @@
  end

  describe '#validate_ticket' do
+    subject(:service) { Cassette.config.service }


You changed the subject here but don't actually use it

rhruiz · 2022-10-10T17:58:14Z

spec/cassette/authentication_spec.rb

@@ -74,23 +80,27 @@
  end

  describe '#validate_ticket' do
+    subject(:service) { Cassette.config.service }
+
+    let(:ticket) { described_class.new }


You don't need another instance of the described_class, the one in the start (with all the instance_double-ed dependencies is fine

rhruiz · 2022-10-10T18:05:05Z

spec/cassette/client/cache_spec.rb


 describe Cassette::Client::Cache do
  it 'uses the cache store set in configuration' do
    # setup
-    global_cache = double('cache_store')
+    global_cache = instance_double(described_class, 'cache_store')


If you want to be thorough, global cache is a ActiveSupport::Cache::Store and not a Cassette::Client::Cache

rhruiz · 2022-10-10T18:05:35Z

spec/cassette/client/cache_spec.rb

@@ -22,14 +18,13 @@
  describe '.backend=' do
    it 'sets the cache' do
      # setup
-      global_cache = double('cache_store')
-      logger = Logger.new('/dev/null')
+      global_cache = instance_double(described_class, 'cache_store')


If you want to be thorough, global cache is a ActiveSupport::Cache::Store and not a Cassette::Client::Cache

rhruiz · 2022-10-10T18:13:09Z

spec/cassette_spec.rb

      end
    end
  end

  describe '.cache_backend' do
    context 'when the cache_backend is already set' do
      it 'returns the cache_backend set' do
-        new_cache = double('cache_backend')
+        new_cache = instance_double(described_class, 'cache_backend')


cache_backend is a ActiveSupport::Cache::Store

rhruiz · 2022-10-10T18:13:20Z

spec/cassette_spec.rb

@@ -123,7 +117,7 @@ def keeping_logger(&block)
    context 'when the cache_backend is not set' do
      it 'returns Rails.cache if set' do
        described_class.cache_backend = nil
-        rails_cache = double('cache')
+        rails_cache = instance_double(described_class, 'cache')


cache_backend is a ActiveSupport::Cache::Store

gleopoldo · 2022-10-10T18:18:35Z

spec/cassette/authentication/user_spec.rb

@@ -8,63 +8,93 @@
  describe '#initialize' do
    context 'without a config' do
      it 'forwards authorities parsing' do
-        expect(Cassette::Authentication::Authorities).to receive(:new).with('[CUSTOMERAPI, SAPI]', nil)
-        Cassette::Authentication::User.new(login: 'john.doe', name: 'John Doe', authorities: '[CUSTOMERAPI, SAPI]')
+        allow(Cassette::Authentication::Authorities).to receive(:new).with('[CUSTOMERAPI, SAPI]', nil)


After this change from expect to allow, is this spec still testing the original behavior? What do you think about using expect(...).to have_received along with this allow, instead? This way you still ensure the original expectancy when the test used expect(...).to receive, but without the rubocop warning :)

gleopoldo · 2022-10-10T18:21:26Z

spec/cassette/authentication/filter_spec.rb

@@ -22,7 +22,7 @@

      it_behaves_like 'with NOAUTH' do
        it 'never checks the role' do
-          expect(current_user).not_to receive(:has_raw_role?)
+          allow(current_user).not_to receive(:has_raw_role?)


Hum... I'm wondering here if this call doesn't make a network call underneath. Maybe we can just safely move to a expect(...).to have_received instead.

Leaving this test as is maybe lead us to false-positives when running rspec.
I wrote down a more complete suggestion in another part of the changes, @toshitapandey - can you see if it applies?

gleopoldo · 2022-10-10T18:30:16Z

spec/cassette/authentication_spec.rb

@@ -38,21 +40,25 @@
    end

    context 'when not cached' do
+      def auth
+        subject
+      end


I didn't understand the need for this function here... wouldn't a simple call to authentication at line 55 match the conditions required by rubocop?

gleopoldo · 2022-10-10T18:37:47Z

spec/integration/cas/client_spec.rb

-    #   end
-    # end
-  end
+      it do


Maybe this won't be possible due to rubocop - it may be checking for "one expect per it" rule. But I'm favorable for adding a description for each it. What about

it "returns an user object` do #... end it "instantiates the returned user with the correct login information" do #... end

toshitapandey added 17 commits October 6, 2022 10:32

add rubocop-rspec gem + require rubocop rspec in rubocop yml

9323a45

add autocorrectable rubocop fixes

211f0d5

resolve rubocop rspec issues in authorities spec

9e5a9e3

resolve rubocop rspec issues in user spec

d2ac852

resolve rubocop rspec issues in cassette spec

3603374

resolve filter spec and client spec

dacca3d

fix rubocop rspec issues in authentication spec

bbc2627

resolve rubocop rspec issues - cache spec

259c7a0

fix rubocop rspec issues - client spec

60da8f2

resolve rubocop rspec - errors + routing constraint spec

fa7b8af

resolve rubocop rspec - cache spec

5ecc322

resolve rubocop rspec - cassette spec

068957f

resolve rubocop rspec - client and null store

7ebb997

resolve rubocop rspec issues - authentication spec

39271d8

resolve rubocop rspec - cache spec

ec82d64

resolve rspec issues - client spec

81fb82e

resolve rubocop rspec issues - client and request spec

aa3beff

toshitapandey changed the title ~~Integrate Rubocop Rspec and Resolve Issues Raised by Rubocop Rspec~~ [WIP] Integrate Rubocop Rspec and Resolve Issues Raised by Rubocop Rspec Oct 8, 2022

toshitapandey changed the title ~~[WIP] Integrate Rubocop Rspec and Resolve Issues Raised by Rubocop Rspec~~ Integrate Rubocop Rspec and Resolve Issues Raised by Rubocop Rspec Oct 8, 2022

rodrigoapereira self-requested a review October 10, 2022 14:14

rodrigoapereira added hacktoberfest hacktoberfest-accepted labels Oct 10, 2022

rodrigoapereira requested changes Oct 10, 2022

View reviewed changes

rhruiz reviewed Oct 10, 2022

View reviewed changes

gleopoldo suggested changes Oct 10, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate Rubocop Rspec and Resolve Issues Raised by Rubocop Rspec #51

Integrate Rubocop Rspec and Resolve Issues Raised by Rubocop Rspec #51

toshitapandey commented Oct 8, 2022

toshitapandey commented Oct 8, 2022

rodrigoapereira commented Oct 10, 2022

rodrigoapereira left a comment

rodrigoapereira Oct 10, 2022

gleopoldo Oct 10, 2022

rodrigoapereira Oct 10, 2022

rodrigoapereira Oct 10, 2022

gleopoldo Oct 10, 2022

rhruiz Oct 10, 2022

toshitapandey Oct 13, 2022

gleopoldo Oct 13, 2022

rhruiz Oct 10, 2022

rhruiz Oct 10, 2022

rhruiz Oct 10, 2022

rhruiz Oct 10, 2022

rhruiz Oct 10, 2022

rhruiz Oct 10, 2022

rhruiz Oct 10, 2022

rhruiz Oct 10, 2022

gleopoldo Oct 10, 2022

gleopoldo Oct 10, 2022

gleopoldo Oct 10, 2022

gleopoldo Oct 10, 2022

Integrate Rubocop Rspec and Resolve Issues Raised by Rubocop Rspec #51

Are you sure you want to change the base?

Integrate Rubocop Rspec and Resolve Issues Raised by Rubocop Rspec #51

Conversation

toshitapandey commented Oct 8, 2022

Solves

Purpose

toshitapandey commented Oct 8, 2022

rodrigoapereira commented Oct 10, 2022

rodrigoapereira 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

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