Refactoring Sandboxing to be in RSpec support #1808
Conversation
|
Why, this sandboxing is only intended to be used in rspec-core.... |
|
This relates to a conversation @myronmarston and I had. We're leveraging sandboxing inside our project to use RSpec to test RSpec code. If you think it should be moved from rspec-support into rspec-core, I am happy to do that. Rspec-support just looked like the obvious place for it. |
|
Ah I see, this code isn't intended for external consumption (read it's far to hacky for us to want to support at the moment), we're fine with you cribbing it and maintaining it yourself in your own project but I'm not ok with extracting this as is into rspec-support. |
|
@JonRowe -- @tyler-ball and I discussed this over skype last week. We've never exposed the sandboxing logic before but given what Chef is going to be doing with RSpec (they are leveraging it as part of chef to allow "audits" of the state of nodes using RSpec's API to specify what's expected) and their need to be able to to test their audit runner (in the same way rspec-core tests itself), they need the same sandboxing thing. We already need to maintain the sandboxing logic anyway so that we can use it, so providing a stable @tyler-ball -- I'd prefer to see the sandboxing logic be placed directly here in rspec-core, given that it's specifically deals with sandboxing rspec-core's world/config state. rspec-support is intended primarily to contain common support code that we'd like to be available to any of rspec-core/expectations/mocks when they are used individually w/o the others. The sandboxing stuff isn't needed by expectations or mocks and really belongs here. I've got some more feedback on it but figure I'll wait until you've ported it back over to here so we can keep the review all in this one PR :). |
|
Okay @myronmarston I moved sandboxing back into rspec-core. Now I'm seeing the same error in Travis that I am seeing locally. I'm guessing this has to do with no longer setting I'll see if I can figure it out. |
| attr_writer :configuration, :world | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Now that we are exposing the sandboxing as a public API, I think we can add these writers to the definition of RSpec in lib/rspec/core.rb. (Just be sure to label them @api private). We had them in the spec stuff because they weren't needed by any public APIs but now that they are they belong in lib/rspec/core.rb, I think.
|
I think it's going to be useful to distinguish between what's essential to the sandbox and what's incidentally needed by rspec-core's specs and was mixed into the sandboxing logic at some point in the past. The incidental stuff probably doesn't belong in I'll comment on the specific things I think don't belong. I don't have time to do that right now (I have to go do something), unfortunately, but will try to do it a few hours from now. |
| class << self | ||
|
|
||
| def sandboxed(&block) | ||
| orig_load_path = $LOAD_PATH.dup |
There was a problem hiding this comment.
The load path isolation is orthogonal to rspec-core sandboxing. It got added because some of rspec-core's specs do load path manipulation, it's useful to keep those mutations in a sandbox and it was simple to add it to the existing sandboxing logic, but I don't think it belongs in the publicly supported rspec-core sandbox.
I'll see if I can figure it out. Let me know if you want to take a stab at that, as it deals with stuff that's pretty orthogonal to what you're working on. Or if you run into trouble with any of the other suggestions I made, let me know and I'm happy to try to help out. |
|
@myronmarston Okay, I updated for all your comments - I still need to fix that example but I don't have time just this second. I'll either do it or ping you with questions - thanks for all the help and great feedback! |
| @@ -43,6 +43,11 @@ module RSpec | |||
|
|
|||
| extend RSpec::Core::Warnings | |||
|
|
|||
| class << self | |||
There was a problem hiding this comment.
I know you don't usually open up the singleton class, but I was getting a private method called error when trying to use self.attr_writer - any idea?
There was a problem hiding this comment.
For macros that you normally use at the class level to define instance methods, but instead want to use to define class methods, you have to the singleton class as you have here, so that's fine. The preference I mentioned before is for normal static method definitions being done using def self.method_name, but for things that really need to run in the context of the singleton class, this is fine.
|
Liking the work so far! |
|
@tyler-ball -- We're hoping to release 3.2 soon and I'd like for this to be included. I completely understand the delay on this (given the holidays) but If you'd like one of us to take it across the finish line, please let us know. |
|
@myronmarston Cleaned up per your comments so far |
| def self.sandboxed(&block) | ||
| raise ArgumentError.new("You must provide a block to be ran in the sandbox") unless block_given? | ||
| raise ArgumentError.new("Your sandboxed block only accepts 1 argument - the config used inside the sandbox") if block.arity > 1 | ||
|
|
There was a problem hiding this comment.
Let me know if I am being too eager here with my condition checking
There was a problem hiding this comment.
Yeah, I think I'd prefer we don't do the block checking here. Ruby's not a statically-typed language and IMO doing these sorts of checks goes against the philosophy of how ruby is designed to be used.
Also, the simple act of capturing &block, even if you yield instead, has a perf cost:
https://github.com/rspec/rspec-core/blob/master/benchmarks/capture_block_vs_yield.rb
...so I'd like to see &block removed entirely.
|
Now lets just hope the specs that pass locally pass in Travis 😄 |
| # from the running RSpec instance - it should return a sandboxed configuration. | ||
| # | ||
| # @param new_config [RSpec::Core::Configuration] A custom config to use when executing the sandboxed tests. | ||
| # Defaults to a default Configuration object. |
There was a problem hiding this comment.
This isn't a param any more.
|
This is looking good, @tyler-ball. I left a couple comments. Please rebase and squash into one commit when you've addressed those and I'll be glad to merge this. BTW, I expect the jruby builds to fail on travis -- they rolled out a new environment today and there are some problems there. |
|
Updated the yarddoc, removed the EDIT Added full yarddoc coverage - didn't realize that was failing the test, I thought it was a warning. Let me know if what I wrote does not make sense. |
tyler-ball
force-pushed
the
master
branch
2 times, most recently
from
aecf200 to
3f4fb6d
Compare
|
@myronmarston 1.8.7 and ree are now the only ones failing because the formatter tests expect a different style - can you provide some guidance? |
| | # ./spec/support/sandboxing.rb:2 | ||
| | # ./spec/support/sandboxing.rb:18:in `block (4 levels) in <top (required)>' | ||
| | # ./spec/support/sandboxing.rb:18:in `block (3 levels) in <top (required)>' | ||
| | # ./spec/support/sandboxing.rb:9:in `block (2 levels) in <top (required)>' |
There was a problem hiding this comment.
On 1.8.7 (which is the context for which this fixture is used to match), these three lines don't have the :in block... bit. That's what's causing the failures on travis.
Backtraces look different on ruby 1.8 so there's a different test fixture defined in |
Adding full yardoc coverage
|
Thanks, @tyler-ball. The appveyor build is a transient faliure that's unrelated to your changes here (and I'm working on a fix for it) so I'm going to merge this. |
Refactoring Sandboxing to be in RSpec support
|
Thanks for cleaning up my code issues and getting this in! |
Refactoring Sandboxing to be in RSpec support
rspec-core changes for #1798
rspec-support changes in rspec/rspec-support#137