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

Remove monkey patching #1036

Merged
merged 16 commits into from Dec 3, 2013
Merged

Remove monkey patching #1036

merged 16 commits into from Dec 3, 2013

Conversation

@JonRowe
Copy link
Member

JonRowe commented Aug 8, 2013

ATM I've defaulted to off in our specs, and on in the cucumbers, but we can discuss that if you disagree.

Thoughts?

/cc @alindeman @myronmarston @samphippen @soulcutter

@JonRowe
Copy link
Member Author

JonRowe commented Aug 12, 2013

Ping! Has anyone had a chance to start to review this yet?

@myronmarston
Copy link
Member

myronmarston commented Aug 12, 2013

I haven't yet...too busy with moving still. I'll try to get to it soon.

One general comment: there's also #870 where some work was started to make example group alias methods available at the top level. Does this PR fit in with that? I'd like to find a way to integrate that at some point.

@JonRowe
Copy link
Member Author

JonRowe commented Aug 13, 2013

I need to rework this to take advantage of #870 me thinks...

@ghost ghost assigned JonRowe Oct 3, 2013
@JonRowe
Copy link
Member Author

JonRowe commented Oct 14, 2013

Ok so I'd like to get this sorted, and then take #870 back on top of this, I don't mind assisting or taking over from @michihuber to get these two across the line.

@JonRowe
Copy link
Member Author

JonRowe commented Oct 14, 2013

So a review would be good, then I'll sort squashing etc, at a minium a change log entry needs adding ;)

@@ -0,0 +1,44 @@
Feature: Monkey patching

Use the monkey patching to tell RSpec to allow or disallow the top level dsl.

This comment has been minimized.

Copy link
@xaviershay

xaviershay Oct 14, 2013

Member

"the monkey patching" is funny, but should just be "monkey patching" to be correct. I don't mind one way or the other :)

This comment has been minimized.

Copy link
@JonRowe

JonRowe Oct 15, 2013

Author Member

I actually wonder if we should call this monkey patching at all, perhaps we should state that what this is doing more explicitly? How about:

Tell RSpec to expose the dsl via the global namespace.

This comment has been minimized.

Copy link
@xaviershay
Scenario: when monkey patch is disabled top level dsl no longer works
Given a file named "config.rb" with:
"""ruby
RSpec.configure { |c| c.enable_monkey_patching = false }

This comment has been minimized.

Copy link
@xaviershay

xaviershay Oct 14, 2013

Member

Should this option just be c.monkey_patching? c.monkey_patch? enable = false is weird.

This comment has been minimized.

Copy link
@JonRowe

JonRowe Oct 15, 2013

Author Member

See previous comment, perhaps we should name this entirely differently, c.expose_globally ?

.rspec Outdated
@@ -1,3 +1,4 @@
--default_path spec
--order rand
--warnings
-r spec_helper

This comment has been minimized.

Copy link
@myronmarston

myronmarston Oct 15, 2013

Member

Why did you add this?

This comment has been minimized.

Copy link
@myronmarston

myronmarston Oct 15, 2013

Member

I understand now: you have to do this so that the config setting that disables monkey patches can be set before loading spec files. I think it creates a burden on our users that this requires a -r option like this, though, so I'd like to use a different mechanism that doesn't require this.

@@ -871,6 +876,9 @@ def configure_expectation_framework

# @private
def load_spec_files
if enable_monkey_patching?
require 'rspec/core/monkey_patch'
end

This comment has been minimized.

Copy link
@myronmarston

myronmarston Oct 15, 2013

Member

Given how small the monkey patch file is, and that it doesn't define any classes or methods, but just calls a few methods, I think it would make more sense to put that code into a private configuration method that is called from here, rather than requiring a file.

@@ -205,6 +205,7 @@ def split_in_half(array)
def run_command(cmd)
in_current_dir do
RSpec::Core::Runner.run(cmd.split, stderr, stdout)
RSpec::configuration.enable_monkey_patching = false

This comment has been minimized.

Copy link
@myronmarston

myronmarston Oct 15, 2013

Member

Ruby allows you to use :: to call class methods, but I prefer the consistency of always doing message sends with .. Any reason you chose :: here?

@@ -13,9 +13,16 @@

methods.each do |method_name|
describe "##{method_name}" do
it "is not added to every object in the system" do
it "is added to the main object and Module when monkey patching is enabled" do
pending "how do we sandbox this?"

This comment has been minimized.

Copy link
@myronmarston

myronmarston Oct 15, 2013

Member

Use in_sub_process to sandbox it. Subprocesses are great for sandboxing :).

describe Identity do
it "does not affect the ordering of the items" do
expect(Identity.new.order([1, 2, 3])).to eq([1, 2, 3])
RSpec.describe do

This comment has been minimized.

Copy link
@myronmarston

myronmarston Oct 15, 2013

Member

Why did you wrap things in an anonymous describe?

This comment has been minimized.

Copy link
@JonRowe

JonRowe Oct 15, 2013

Author Member

It seemed easier than changing all three describes, I can swap it back.

This comment has been minimized.

Copy link
@myronmarston

myronmarston Oct 16, 2013

Member

Please do.

@@ -38,10 +38,15 @@ module SharedExampleGroup
group.send(shared_method_name, *args, &block)
end

it "is exposed to the global namespace" do
it "is exposed to the global namespace when monkey patching is enabled" do
pending "how do we sandbox this"

This comment has been minimized.

Copy link
@myronmarston

myronmarston Oct 15, 2013

Member

in_sub_process.

@myronmarston
Copy link
Member

myronmarston commented Oct 15, 2013

The difficulty is finding "when" to do it, as I don't think it's feasible to remove monkey patching after its happened, so we need to do it at the last possible minute (before spec files are loaded).

You can always undefine the monkey-patched methods as we do in rspec-mocks and rspec-expectations, but that would require bare method defs rather than module inclusion/extension. And actually, I think I prefer the way you have it.

Hmm, actually, thinking about it some more: I'm not sure that the way you have it will work for end users. You add the monkey patches before loading the spec files. Most users who will disable this will disable it in spec_helper.rb, which will not be loaded and evaluated until load_spec_files loads a file that requires it.

So I think we probably want to move to a mechanism like rspec-mocks and rspec-expectatations. See the syntax files there.

Overall this is looking great. I want to hold off on merging this until we release our 3.0.0.alpha, though.

@JonRowe
Copy link
Member Author

JonRowe commented Oct 20, 2013

Okay, this has been reworked to change the name to expose_globally which defaults to true, and will enable / disable the top level stuff whenever it's invoked (no need for presetting)

@JonRowe
Copy link
Member Author

JonRowe commented Oct 20, 2013

Build failure is due to @alindeman's JRuby issue in rspec-mocks :/


Tell RSpec to expose the DSL via the global namespace

Scenario: by default RSpec allows the top level monkey patching

This comment has been minimized.

Copy link
@xaviershay

xaviershay Oct 21, 2013

Member

should update this text to enable global.

@xaviershay
Copy link
Member

xaviershay commented Oct 21, 2013

I want to hold off on merging this until we release our 3.0.0.alpha, though.

@myronmarston why?

Scenario: when exposing globally is disabled top level dsl no longer works
Given a file named "spec/example_spec.rb" with:
"""ruby
RSpec.configure { |c| c.expose_globally = false }

This comment has been minimized.

Copy link
@myronmarston

myronmarston Oct 21, 2013

Member

You know, I'm not sure that expose_globally has the right sense. It's not exposed in the global scope like a global variable is. It's exposed at the top level scope off of main.

Also, from the config option name it's not clear what is being exposed.

What do you think about expose_dsl_off_of_main instead?

This comment has been minimized.

Copy link
@JonRowe

JonRowe Oct 21, 2013

Author Member

It's not just main though, it's also off Module (so you can nest describe inside modules/classes) so it is kind of globally... it's also too wordy for my liking.

This comment has been minimized.

Copy link
@JonRowe

JonRowe Oct 21, 2013

Author Member

I could go for expose_dsl though.

@myronmarston
Copy link
Member

myronmarston commented Oct 21, 2013

@xaviershay -- my primary concern is that I want a release to be consistent, without in-progress, half-done features. #870 is another aspect of this feature, I think, and it feels incomplete without that. As such, I don't want to have this merged and then block our being able to release a solid alpha. Once we ship that first alpha, it's fine to merge things like this that will have later PRs to round-out the features.

@michihuber
Copy link
Contributor

michihuber commented Oct 21, 2013

@JonRowe, I'll keep an eye on this PR and resubmit #870 once this is ready for master.

@JonRowe
Copy link
Member Author

JonRowe commented Nov 10, 2013

So I rebased this and it broke horrifically. I'm at a loss as to what's changed in master that breaks this, my hunch is that it's something to do with the sandboxing going awry... See the diff or pull down the no_monkey_patch_rebased branch and run for yourself o_O

e.g. HALP

/cc @myronmarston @xaviershay @samphippen

@JonRowe
Copy link
Member Author

JonRowe commented Nov 14, 2013

Did anyone get a chance to look at my other branch? @myronmarston @xaviershay @samphippen

@myronmarston
Copy link
Member

myronmarston commented Nov 14, 2013

Not yet -- I'll take a look later this week.

@myronmarston
Copy link
Member

myronmarston commented Nov 15, 2013

I pulled your branch now and bisected it. b9adedd is the source commit that introduces the problems.

@JonRowe
Copy link
Member Author

JonRowe commented Nov 15, 2013

Yes, it's also the first commit commit that turns off monkey patching on this and that branch. However it hasn't changed since before I rebased it, where it works fine. There seem to be no related changes, the commit actually causing the failure lies between the point where I originally branched off and the current master.

My investigations have lead me to uncover that somehow run is being undefined from ExampleGroup subclasses or the subclasses themselves are not inheriting changes properly. My assumption is that this is somehow related to no longer mixing in our classes into the top level namespace.

@myronmarston
Copy link
Member

myronmarston commented Nov 20, 2013

@JonRowe -- would you like me to take this over or are you still planning to get to the bottom of the problem you're having?

@JonRowe
Copy link
Member Author

JonRowe commented Dec 2, 2013

FYI the methods are idempotent just Ruby issues a warning... additionally I've actually noticed the methods protect against this themselves making the configuration check a non issue. I've also moved the initial call to the only place we initialised Configuration. So by default it'll be set but for every other usage of Configuration.new it won't.

I personally read expose_globally as RSpec.expose_globally so it's clear to me, willing for other voices? @samphippen @xaviershay @soulcutter

config_1 = Configuration.new
config_2 = Configuration.new
config_1 = Configuration.new.tap { |c| c.expose_globally = false }
config_2 = Configuration.new.tap { |c| c.expose_globally = false }

This comment has been minimized.

Copy link
@myronmarston

myronmarston Dec 2, 2013

Member

Given that new Configuration instances no longer set expose_globally = true, I think you no longer have to set expose_globally = false here (or in any of the other specs), right?

This comment has been minimized.

Copy link
@JonRowe

JonRowe Dec 2, 2013

Author Member

I think you're right, shall we drop them?

This comment has been minimized.

Copy link
@myronmarston

myronmarston Dec 2, 2013

Member

Please do. It's noise and it suggests that it's necessary when it's not, which leads to confusion.

@myronmarston
Copy link
Member

myronmarston commented Dec 2, 2013

FYI the methods are idempotent just Ruby issues a warning

Since we want rspec to be warning-free, can you fix it so ruby doesn't issue a warning?

I've also moved the initial call to the only place we initialised Configuration. So by default it'll be set but for every other usage of Configuration.new it won't.

👍

I personally read expose_globally as RSpec.expose_globally so it's clear to me, willing for other voices?

That doesn't seem any clearer to me: RSpec.expose_globally is telling RSpec to expose something globally but it doesn't say what should be exposed.

If others think it's perfectly clear I'm happy to go with what you have, though.

@JonRowe
Copy link
Member Author

JonRowe commented Dec 2, 2013

FYI the methods are idempotent just Ruby issues a warning

Since we want rspec to be warning-free, can you fix it so ruby doesn't issue a warning?

They are warning free, thats what the extra protective code is doing, I removed the var check from Configuration but it seems I'm also doing it in DSL so it's all fine.

@myronmarston
Copy link
Member

myronmarston commented Dec 2, 2013

They are warning free, thats what the extra protective code is doing, I removed the var check from Configuration but it seems I'm also doing it in DSL so it's all fine.

👍 I guess I misread your comment then. Thanks!

Anyhow, this is looking good to me. My concern about the expose_globally name remains but that'll really come down to what others think (@xaviershay? @alindeman? @samphippen? @soulcutter?). This also needs a changelog, then merge away.

@myronmarston
Copy link
Member

myronmarston commented Dec 2, 2013

Oh yeah, one other thing: the expose_globally config option could use some YARD doc comments explaining what it does. (But we can always address that in a separate PR; don't let that hold up the merge here).

@JonRowe
Copy link
Member Author

JonRowe commented Dec 2, 2013

I added an additional line to the YARD explaining a bit more, I also changed my mind about the name of the config var.

@JonRowe
Copy link
Member Author

JonRowe commented Dec 2, 2013

I've moved the config line back to the sandboxing. My reasoning is that this is the easiest way to ensure consistent state across the spec run with regards to this setting. To me it makes much more sense to protect against this one setting here rather than run anything that touches RSpec.reset or RSpec.run in a sub process... WDYT @myronmarston?

JonRowe added a commit that referenced this pull request Dec 3, 2013
Remove monkey patching
@JonRowe JonRowe merged commit 9a1deb3 into master Dec 3, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@JonRowe JonRowe deleted the no_monkey_patch branch Dec 3, 2013
@JonRowe
Copy link
Member Author

JonRowe commented Dec 3, 2013

I decided if we need to refactor the specs thwn we can do that later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.