New #stub_user controller macro #10243

chrisarcand · 2016-08-03T22:27:44Z

This replaces the old #set_user_privileges method.

Besides being awesome that you don't need to remember how to spell 'privileges', this replacement method is much more flexible and correct than the previous one.

It is the groundwork for forthcoming role checking changes and reduces the diff substantially.

@miq-bot add_labels tenancy, core, darga/no
@miq-bot assign @Fryguy
cc/ @gtanzillo @dclarizio

jrafanie · 2016-08-03T22:29:12Z

spec/support/controller_spec_helper.rb

-    # TODO: remove these stubs
-    allow(controller).to receive(:check_privileges).and_return(true)
-    login_as user
-    allow_any_instance_of(User).to receive(:role_allows?).and_return(true)


Why it's wrong: it accepts custom user with features as input but blindly says "yeah", you can do whatever you want.

The new stub_user, on the other hand, allows you to create an (features: :all) "permitted" or "denied"(features: :none) user. This accounts for the majority of test cases. When you want to test specific features, you should use login_as user_with_feature(...)

We could potentially do stubbing of specific features (that's why the API here is so explicit, so if we run into a reason to do so someday...), but that's not something we're interested in doing quite yet.

jrafanie · 2016-08-03T22:36:20Z

@gtanzillo @dclarizio this is what was required in the tests for us to funnel the AppHelper#role_allows, User#role_allows, etc. through one Rbac class.

jrafanie · 2016-08-03T22:39:09Z

spec/controllers/provider_foreman_controller_spec.rb

@@ -95,7 +95,7 @@
    end

    it "renders explorer based on RBAC access to feature 'provider_foreman_add_provider'" do
-      set_user_privileges user_with_feature %w(provider_foreman_add_provider)
+      login_as user_with_feature %w(provider_foreman_add_provider)


We should now use login_as user_with_feature to test specific features. We don't want to stub things when we're testing specific features as OTHER features would get that stubbed response.

In other terms, we should use login_as when you want a real user model, saved to the db, and stub_user when you want a user object with stubbed responses (stub_user currently creates a real user model but I'd soon like to try using an RSpec test double or a FactoryGirl object (build, not create))

This is a distinction I'd really like to push, as we can avoid people doing db writes for everything just for a sake of a user object when they can just stub it.

I've added what I mention above here in 9af8717 - see the commit message there for details.

chrisarcand · 2016-08-04T02:21:06Z

@Fryguy @bdunne As you can see, we really need to write a new bot rule to omit yelling about hash rockets syntax in the case of Ruby 2.0+ keyword arguments. 😕

chrisarcand · 2016-08-04T14:13:31Z

The previous build error (that is now green, re-running it) came from an unrelated issue recently merged to master

Fryguy · 2016-08-04T18:09:36Z

We need to add TargetRubyVersion to the .rubocop.yml to avoid the crash boom bang ... Can you do it in this PR, and set it to Ruby 2.2?

AllCops:
  TargetRubyVersion: "2.2"

Fryguy · 2016-08-04T18:11:10Z

spec/controllers/application_controller/automate_spec.rb

@@ -1,7 +1,7 @@
 describe MiqAeCustomizationController, "ApplicationController::Automate" do
  context "#resolve" do
    before(:each) do
-      set_user_privileges
+      stub_user(features: :all)


Look at you trying to sneak in the non hash-rockets

Kwargs to the rescue!

In all honesty though, these should all be hash rockets...from the caller side, it's a regular hash, not kwargs. kwargs syntax at the signature level, which is what it was designed for, is required: def stub_user(features:) is required syntax (vs def stub_user(:features =>) which is invalid syntax)

Ahh I forgot that's true. Ok

And yes, I know you have a handy script and are just trolling me

Fryguy · 2016-08-04T18:30:42Z

Overall 👍 LGTM except the minor comments above.

Replaces #set_user_privileges This consolidates the stubbing of users in controllers specs to a single, flexible, more controlled spot. It enables easier integration of forthcoming tenancy changes (multiple entitlements/roles)

With the addition of #stub_user, this [very janky, barely works] method can be removed.

This changes stub_user to use a non-persisted user object and changes the :user_with_group factory to use a non-persisted group object. This increases test performance as stub_user is called over 1,000 times in controller specs, writing a user and group record to the database every time. Automation specs also make heavy use of :user_with_group, so each group write there is avoided. As expected, stubbing is more performant but loses a little bit of true parity with actual code testing. For example, because the user and group are empty objects and have no primary keys, code that utilizes IDs directly MUST use a real record. For that, one should use create an actual user/group and use 'login_as'. Given that I needed to change only a *single* spec because of this, it seems well worth it in 99% of cases (the spec I have changed here uses current_user.miq_groups.ids in a query for groups added to a report) In short, stub_user vs. login_as should be used as most people are told to use FactoryGirl's create vs. build - Use build as much as you can, but create if you really need to.

Avoids the 💣 💥 🔥 🚒

miq-bot · 2016-08-04T20:07:23Z

Checked commits chrisarcand/manageiq@5e91d1d~...fbdabcf with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
78 files checked, 2 offenses detected

spec/helpers/application_helper_spec.rb

❗ - Line 65, Col 15 - Style/CommentAnnotation - Annotation keywords like TODO should be all upper case, followed by a colon, and a space, then a note describing the problem.

spec/support/auth_helper.rb

❕ - Line 14, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for stub_user is too high. [38.11/15]

miq-bot added core/tenancy core darga/no labels Aug 3, 2016

miq-bot assigned Fryguy Aug 3, 2016

jrafanie reviewed Aug 3, 2016
View reviewed changes

chrisarcand force-pushed the new-stub-user-api branch from 9af8717 to 9f48a19 Compare August 4, 2016 13:05

Fryguy reviewed Aug 4, 2016
View reviewed changes

chrisarcand force-pushed the new-stub-user-api branch from 12d350f to d203594 Compare August 4, 2016 18:38

chrisarcand added 3 commits August 4, 2016 13:57

New #stub_user controller helper

5e91d1d

Replaces #set_user_privileges This consolidates the stubbing of users in controllers specs to a single, flexible, more controlled spot. It enables easier integration of forthcoming tenancy changes (multiple entitlements/roles)

Remove set_user_privileges ✂️

1ac4516

With the addition of #stub_user, this [very janky, barely works] method can be removed.

chrisarcand force-pushed the new-stub-user-api branch from d203594 to 2b8420a Compare August 4, 2016 18:57

Set rubocop TargetRubyVersion to 2.2

fbdabcf

Avoids the 💣 💥 🔥 🚒

jrafanie force-pushed the new-stub-user-api branch from 2b8420a to fbdabcf Compare August 4, 2016 20:01

Fryguy merged commit f8cfcbc into ManageIQ:master Aug 4, 2016

Fryguy added this to the Sprint 45 Ending Aug 22, 2016 milestone Aug 4, 2016

Fryguy added the test label Aug 4, 2016

chrisarcand mentioned this pull request Aug 5, 2016

Introduce Rbac::Authorizer and top level role_allows? in AH and User to it #10284

Merged

chrisarcand deleted the new-stub-user-api branch October 5, 2016 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New #stub_user controller macro #10243

New #stub_user controller macro #10243

chrisarcand commented Aug 3, 2016 •

edited

jrafanie Aug 3, 2016

jrafanie Aug 3, 2016

chrisarcand Aug 4, 2016

jrafanie commented Aug 3, 2016

jrafanie Aug 3, 2016

chrisarcand Aug 4, 2016

chrisarcand Aug 4, 2016

chrisarcand commented Aug 4, 2016

chrisarcand commented Aug 4, 2016

Fryguy commented Aug 4, 2016 •

edited

Fryguy Aug 4, 2016

chrisarcand Aug 4, 2016

Fryguy Aug 4, 2016

chrisarcand Aug 4, 2016

Fryguy Aug 4, 2016

chrisarcand Aug 4, 2016

Fryguy commented Aug 4, 2016

miq-bot commented Aug 4, 2016

New #stub_user controller macro #10243

New #stub_user controller macro #10243

Conversation

chrisarcand commented Aug 3, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Aug 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisarcand commented Aug 4, 2016

chrisarcand commented Aug 4, 2016

Fryguy commented Aug 4, 2016 • edited

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

Fryguy commented Aug 4, 2016

miq-bot commented Aug 4, 2016

chrisarcand commented Aug 3, 2016 •

edited

Fryguy commented Aug 4, 2016 •

edited