Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve examples for LeakyConstantDeclaration cop #779

Conversation

@pirj
Copy link
Member

commented Jun 20, 2019

As mentioned here, it's not even necessary in some cases to stub a constant, a so-called "sacrificial class" will do.
The term was coined in this article originally mentioned in this comment.

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • [-] Added tests.
  • [-] Added an entry to the changelog if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).
@pirj

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

@dgollahon Based on our discussion here.

@dgollahon
Copy link
Member

left a comment

I think these are good improvements 👍

I've left some minor wording suggestions, but as written is an improvement so don't feel obligated to apply all my feedback if you disagree.

lib/rubocop/cop/rspec/leaky_constant_declaration.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/leaky_constant_declaration.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/leaky_constant_declaration.rb Outdated Show resolved Hide resolved

@pirj pirj force-pushed the pirj:improve-examples-for-leaky-constant-declaration-cop branch 2 times, most recently from 6a920da to d247c8a Jun 23, 2019

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jun 23, 2019

Thanks for the review @dgollahon, all notes make sense to me.

@pirj pirj force-pushed the pirj:improve-examples-for-leaky-constant-declaration-cop branch from d247c8a to 10ec383 Jun 28, 2019

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

Force-pushed to trigger CI.

Is there anything that needs to be adjusted here @bquorning @Darhazer ?

@bquorning

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

I think GitHub or CircleCI is confused because you have set up https://circleci.com/gh/pirj/rubocop-rspec (as I have set up https://circleci.com/gh/bquorning/rubocop-rspec). Sometimes it works, sometimes it doesn’t – I can’t really figure it out.

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

I suppose they should have support for building forks.
If there's really that problem, I can try removing my fork from CircleCI.
What's confusing is that each build should report to its fork's repo, not a randomly picked one.
Let me trigger the build once again, and see if it reproduces.

Improve examples for LeakyConstantDeclaration cop
As mentioned here
#765 (comment),
it's not even necessary in some cases to stub a constant, a so-called
"sacrificial class" will do.
Originally mentioned article
http://blog.bitwrangler.com/2016/11/10/sacrificial-test-classes.html in
this comment #197 (comment)

@pirj pirj force-pushed the pirj:improve-examples-for-leaky-constant-declaration-cop branch from 10ec383 to 5808b7b Jun 28, 2019

@bquorning

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

👍

Meanwhile, I’ll check the settings on the main repo CircleCI (https://circleci.com/gh/rubocop-hq/rubocop-rspec)

@bquorning

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

These are the settings on https://circleci.com/gh/rubocop-hq/rubocop-rspec. If anything, I would expect pull requests from forks to be built here, not on the fork owner’s CI…

Screenshot 2019-06-28 at 23 24 55

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

That's strange. There were no builds since 8 days ago.
Filed support ticket on CircleCI https://support.circleci.com/hc/en-us/requests/53642

@bquorning bquorning merged commit 4fa3d7f into rubocop-hq:master Jun 29, 2019

@bquorning

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

I ran bundle exec rake locally, against your branch, and of course everything passed.

Using my admin privileges to override the usual “green build” merge restriction. 🚀

@pirj pirj deleted the pirj:improve-examples-for-leaky-constant-declaration-cop branch Jun 29, 2019

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2019

Thanks!

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

@bquorning From CircleCI:


Hello Phil,

Thank you for reaching out to CircleCI Support!

Can you take a look at the Github Webhook deliveries for this project? We would like to see if webhooks are being delivered or if there is some other issue.

You can find more information on viewing your Github Webhook deliveries here: How to view your GitHub WebHook deliveries

Please let us know if you have any more questions, happy building!

Scott Wheeler
Support Engineer @ CircleCI


@bquorning

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

It seems webhooks are working fine. All pull requests where people don’t have their own setup of circleci works fine – the webhooks cause builds to be run on circleci.com/gh/rubocop-hq/rubocop-rspec/

You can tell them to look for the synchronize webhook with X-GitHub-Delivery: d434ae10-99ea-11e9-89df-94c15a574108 which was emitted at "2019-06-28T21:22:27Z" when you force-pushed commit 5808b7b.

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

@bquorning It seem that the problem was because I followed my fork of rubocop-rspec project on CircleCI from my personal account there. As soon as I unfollowed it, the builds on my pulls started triggering.

A support article on this topic.

To unfollow a project, go to https://circleci.com/add-projects/gh/[YOUR-GITHUB-USERNAME]
Find the fork of the project and click Unfollow.

@bquorning

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

I had no idea, but that is indeed good to know. Thanks for sharing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.