Add a spec for the project policy #5878

vpereira · 2018-09-19T13:07:20Z

Add rspec for the pundit project policy

Ana06 · 2018-09-19T13:29:39Z

src/api/spec/policies/project_policy_spec.rb

+  end
+
+  permissions :local_project_and_allowed_to_create_package_in? do
+    it { expect(subject).to permit(user, project) }


this test is repeated! 🙈 it is one case for line 13. Instead of disabling Rubocop, listen to the smart Rubocop and remove this line 😉

they are repeated or rubocop cannot understand the permissions block? For me looking the documentation https://www.rubydoc.info/gems/pundit/Pundit%2FRSpec%2FDSL:permissions - and looking the source code, look like we are testing different method, with the same parameters, which results in different tests, therefore not repeated, no @Ana06 ?

no, as I said, rubocop doesn't understand the permission block. Not so smart ;)

Ana06 · 2018-09-19T15:41:05Z

@vpereira

they are repeated or rubocop cannot understand the permissions block? For me looking the documentation https://www.rubydoc.info/gems/pundit/Pundit%2FRSpec%2FDSL:permissions - and looking the source code, look like we are testing different method, with the same parameters, which results in different tests, therefore not repeated, no @Ana06 ?

no, as I said, rubocop doesn't understand the permission block. Not so smart ;)

it was repeated, but you are right there is also a Rubocop bug. When having a Rubocop bug instead of disabling it in the file, where it will keep for ever even if it gets fixed upstream, you should report it upstream and regenerate the todo file. This way if it is solved upstream, we don't have to do anything else. Reporting it upstream is IMO part of the culture/philosophy of working in open source, we should contribute with the upstream project we are using as it is good for us as well and because we want our users to do the same and it takes 1 minute to add a comment to the issue. Anyway, as you said you don't want to report it, I have done already: rubocop/rubocop-rspec#333 (comment)

Ana06 · 2018-09-19T15:42:19Z

src/api/spec/policies/project_policy_spec.rb

+  end
+
+  permissions :local_project_and_allowed_to_create_package_in? do
+    it { expect(subject).not_to permit(user, '') }


I think we should test this also for other kind of users 🤔 I mean the case with an empty string.

not necessary. There is no different behavior if it's a remote project and user is normal or user is admin. Unless you spot some specific use case. Otherwise I would ask you to approve it,

ping @Ana06 could you please tell me if you see a different use case or please accept it? thank you.

@vpereira

Otherwise I would ask you to approve it,

@Ana06 could you please tell me if you see a different use case or please accept it? thank you.

We don't need to hurry to approve it. I have some more concerns. I'll add them in a moment

As @mdeniz mentioned, this is private and shouldn't now be tested, for any user

vpereira · 2018-09-19T18:28:09Z

Reporting it upstream is IMO part of the culture/philosophy of working in open source,

sure, please go ahead. Even better if you submit a PR to them. Thank you for making the open source community better 💓 💓 💓 💓

hennevogel · 2018-09-21T11:53:53Z

@Ana06? 2 Days hanging now.

mdeniz

I think that you need to cover update? and destroy?. I would cover also index? and show? to ensure that always return true.

mdeniz · 2018-09-21T12:01:14Z

src/api/spec/policies/project_policy_spec.rb

+    it { expect(subject).to permit(admin_user, project) }
+  end
+
+  permissions :local_project_and_allowed_to_create_package_in? do


This is a private method in the policy, no need to test it

It was probably not private when @vpereira wrote the test, as this was introduced in 3e22695

It makes now no sense to test it.

it wasn't private, and show? and index? didn't exist @mdeniz

mdeniz · 2018-09-21T12:01:40Z

src/api/spec/policies/project_policy_spec.rb

+  let(:project) { user.home_project }
+  subject { ProjectPolicy }
+
+  permissions :create?, :can_create_package_in?, :unlock?, :local_project_and_allowed_to_create_package_in? do


local_project_and_allowed_to_create_package_in? is private, no need to test it

Ana06 · 2018-09-21T12:06:12Z

@hennevogel

@Ana06? 2 Days hanging now.

There has been some recent changes (some methods are now private) that affect this PR, I need to review this again. @mdeniz has just commented about that.

Ana06

I agree with @mdeniz that the untested method should be tested, specially update because it does include a lot of logic.

Ana06 · 2018-09-21T12:19:38Z

src/api/spec/policies/project_policy_spec.rb

+  end
+
+  permissions :local_project_and_allowed_to_create_package_in? do
+    it { expect(subject).not_to permit(user, '') }


As @mdeniz mentioned, this is private and shouldn't now be tested, for any user

Ana06 · 2018-09-21T12:21:03Z

src/api/spec/policies/project_policy_spec.rb

+
+  permissions :create?, :can_create_package_in?, :unlock?, :local_project_and_allowed_to_create_package_in? do
+    it { expect(subject).to permit(user, project) }
+    it { expect(subject).not_to permit(anonymous_user, project) }


I think in the test here we should test the code in project_policy.rb, but not the code in can_create_project? and can_modify? which should be tested in the User model. So basically I would have just 2 test, one with a user and another one without user. The rest is in IMO responsibility of the model test.

Ana06 · 2018-09-24T08:34:23Z

@vpereira why have you closed this? 😕

vpereira · 2018-09-24T10:39:07Z

@Ana06 this was a simple PR, that should be merged before 3e22695, but then it got stuck in the review.

if it was approved and merged before the 3e22695, then tests for local_project_and_allowed_to_create_package_in? would fail, @hennevogel would have to adapt it, extend it to cover the new permissions added by him. It would be a quick win.

However looks like there were more important issues "to be resolved" here before it get merged.. just sad 💔

Ana06 suggested changes Sep 19, 2018

View reviewed changes

Add a spec for the project policy

d1e597e

vpereira force-pushed the project_policy_spec branch from d383aca to d1e597e Compare September 19, 2018 15:34

Ana06 suggested changes Sep 19, 2018

View reviewed changes

DavidKang added Frontend Things related to the OBS RoR app Test Suite / CI 💉 Things related to our tests/CI labels Sep 20, 2018

mdeniz suggested changes Sep 21, 2018

View reviewed changes

Ana06 suggested changes Sep 21, 2018

View reviewed changes

vpereira closed this Sep 22, 2018

pirj mentioned this pull request Nov 6, 2020

Add docs and provide a reference implementation on RSpec DSL aliases configuration in third-party gems rubocop/rubocop-rspec#1077

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a spec for the project policy #5878

Add a spec for the project policy #5878

vpereira commented Sep 19, 2018

Ana06 Sep 19, 2018

vpereira Sep 19, 2018 •

edited

Loading

vpereira Sep 19, 2018

Ana06 commented Sep 19, 2018 •

edited

Loading

Ana06 Sep 19, 2018 •

edited

Loading

vpereira Sep 19, 2018

vpereira Sep 21, 2018

Ana06 Sep 21, 2018 •

edited

Loading

Ana06 Sep 21, 2018

vpereira commented Sep 19, 2018 •

edited

Loading

hennevogel commented Sep 21, 2018

mdeniz left a comment

mdeniz Sep 21, 2018

Ana06 Sep 21, 2018 •

edited

Loading

vpereira Sep 21, 2018

mdeniz Sep 21, 2018 •

edited

Loading

Ana06 commented Sep 21, 2018 •

edited

Loading

Ana06 left a comment

Ana06 Sep 21, 2018

Ana06 Sep 21, 2018

Ana06 commented Sep 24, 2018

vpereira commented Sep 24, 2018

Add a spec for the project policy #5878

Add a spec for the project policy #5878

Conversation

vpereira commented Sep 19, 2018

Choose a reason for hiding this comment

vpereira Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ana06 commented Sep 19, 2018 • edited Loading

Ana06 Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ana06 Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpereira commented Sep 19, 2018 • edited Loading

hennevogel commented Sep 21, 2018

mdeniz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ana06 Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdeniz Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Ana06 commented Sep 21, 2018 • edited Loading

Ana06 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ana06 commented Sep 24, 2018

vpereira commented Sep 24, 2018

vpereira Sep 19, 2018 •

edited

Loading

Ana06 commented Sep 19, 2018 •

edited

Loading

Ana06 Sep 19, 2018 •

edited

Loading

Ana06 Sep 21, 2018 •

edited

Loading

vpereira commented Sep 19, 2018 •

edited

Loading

Ana06 Sep 21, 2018 •

edited

Loading

mdeniz Sep 21, 2018 •

edited

Loading

Ana06 commented Sep 21, 2018 •

edited

Loading