Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Require option regression / Deprecate Configuration#requires= #867

Merged
merged 5 commits into from

3 participants

@soulcutter
Collaborator

Fixes #850 relating to a regression from #831

The cuke probably is better for catching this regression without describing exact implementation (and is probably worthwhile documentation to boot).

@soulcutter
Collaborator

Hm, looks like deferring the require means you can't use it for a custom formatter. May have to move the load path earlier...

@myronmarston

This looks like a good approach for the deferring strategy you decided upon...but I think your last comment is right that it'll probably work better to move the load path setup earlier.

soulcutter added some commits
@soulcutter soulcutter Moves where load path setup and requires happen
* Fixes --require commandline option regression from #831
* Add cucumber documentation for --require
76d84ad
@soulcutter soulcutter Add changelog entry for --require regression 7445ab1
@soulcutter
Collaborator

I moved both the load path and requires up into configuration_options.rb. This seems to get the ordering right.

@JonRowe
Owner

That will mean you can't set the requires via the configuration at all surely? I think some people might be using it in this way... but if this is merged ping me and I'll adapt #834 accordingly.

@soulcutter
Collaborator
@JonRowe
Owner

Specifically I'm thinking of:

RSpec.configure do |config|
  config.requires = some_files
end

but I've re-read the diff and that'd still be possible, you've just marked it as private docs wise. I'm only mentioning it because someone did bring up something along these lines with a rerun tool they were using, so I'm also thinking other tools might use this sort of thing. It was more of a FYI kind of point than an objection ;)

@soulcutter
Collaborator

The more I think about it, the more I think that the configuration syntax should be deprecated while leaving the commandline option. The configuration syntax used to be equivalent to just requiring, which doesn't seem necessary.

features/command_line/require_option.feature
@@ -0,0 +1,21 @@
+Feature: require option
+
+ Use the --require (or -r) option to specify a file in the
+ RSpec load path to require before running specs.
@myronmarston Owner

There's no such thing as "the rspec load path", and I think it might confuse users to suggest there is one. I think it's sufficient to say:

Use the --require (or -r) option to specify a file to require before running specs.

Anyone who knows what the ruby load path is will understand that's how require works, and there's no need to mention it.

(Also, notice my use of backticks: this helps it render better on relish).

@soulcutter Collaborator

Ah, yeah, true about the load path. I just had it on my mind because I was writing this to ensure that the load path got setup properly before requiring.

I think I took the example_name_option.feature file as an example as far as the backticks go. The features aren't entirely consistent... in any case, I will fix as per your comments.

@myronmarston Owner

The features aren't entirely consistent... in any case, I will fix as per your comments.

Nope. That's what happens when they are written by many people over the course of many years :). In any case, code identifiers render better on relish with backticks so it's a good idea to try to remember to use them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/rspec/core/configuration.rb
((8 lines not shown))
RSpec::Core::RubyProject.add_to_load_path(*directories)
end
# @private
+ def load_require_options
@myronmarston Owner

This reads a little funny to me. What about load_requires, load_configured_requires, load_request_requires, or something similar?

Actually, now that I think about it, maybe load shouldn't even be in the method name, given that ruby has both load and require and they do different things -- so maybe perform_requires is a good name? Thoughts?

@soulcutter Collaborator

Agreed about it reading funny. Naming is one of the hardest problems :wink:

My recent refactor actually changes this from load_require_options to setup_load_path_and_require(paths) -- a little verbose, but I think it reads a bit better. Combo names like this can be a odd, but I settled on the idea that you should not be doing one without the other. Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
features/command_line/require_option.feature
((6 lines not shown))
+
+ Scenario: using the --require option
+ Given a file named "spec/foobarator.rb" with:
+ """ruby
+ class Foobarator; end
+ """
+ And a file named "spec/foobarator_spec.rb" with:
+ """ruby
+ describe Foobarator do
+ it "exists" do
+ expect(defined?(Foobarator)).to be_true
+ end
+ end
+ """
+ When I run `rspec --require foobarator`
+ Then the output should contain "1 example, 0 failures"
@myronmarston Owner

I like getting this covered by an acceptance cuke, so nice work here :).

I think the defined?(Foobarator) thing seems a bit convoluted; plus it doesn't really demonstrate the kind of case where you would actually use the --require option (as this spec file would never be able to be loaded w/o it; otherwise describe Foobarator would blow up).

I personally tend to use --require to make some optional tooling available--such as simplecov or a debugger. Can you think of a good way to demonstrate something like that? Like maybe add a file that when loaded causes the formatter output to be split between STDOUT and a file (kinda like tee)? (that's not the best idea, but it's the first thing that came to mind).

@soulcutter Collaborator

Yeah, it's completely convoluted, agreed. Something to illustrate a "useful" reason to use this would be much better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@myronmarston

This looks really solid, @soulcutter -- thanks for taking care of this!

@soulcutter
Collaborator

It's entirely possible that I'm missing a good reason not to deprecate RSpec::Core::Configuration#requires= -- if so, a lot of the refactoring in 0538549 still can be salvaged by simply removing the deprecation warning.

@myronmarston

This looks fine to me. Do you want to update the cucumber scenario to be less convoluted before we merge?

@soulcutter
Collaborator

Yeah, I'd like to update the cuke and add the deprecation to the changelog before merging. I think the core code is done, though.

@soulcutter
Collaborator

All set. The --require cuke is now a bit less contrived, and the deprecation has been added to the changelog.

@myronmarston myronmarston merged commit 5200ad6 into from
@soulcutter soulcutter deleted the branch
@samphippen samphippen referenced this pull request
Closed

Regression from #831 #850

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 11, 2013
  1. @soulcutter

    Moves where load path setup and requires happen

    soulcutter authored
    * Fixes --require commandline option regression from #831
    * Add cucumber documentation for --require
  2. @soulcutter
Commits on Apr 12, 2013
  1. @soulcutter
  2. @soulcutter
Commits on Apr 13, 2013
  1. @soulcutter

    Improve --require docs with something more useful

    soulcutter authored
    Add deprecation to changelog
This page is out of date. Refresh to see the latest.
View
5 Changelog.md
@@ -22,6 +22,8 @@ Enhancements
(Sam Phippen)
* Support `{a,b}` shell expansion syntax in `--pattern` option
(Konstantin Haase).
+* Add cucumber documentation for --require command line option
+ (Bradley Schaefer)
Bug fixes
@@ -49,6 +51,9 @@ Deprecations
`Configuration#backtrace_exclusion_patterns` for greater consistency
and symmetry with new `backtrace_inclusion_patterns` config option
(Sam Phippen).
+* Deprecate `Configuration#requires=` in favor of using ruby's
+ `require`. Requires specified by the command line can still be
+ accessed by the `Configuration#require` reader. (Bradley Schaefer)
### 2.13.1 / 2013-03-12
[full changelog](http://github.com/rspec/rspec-core/compare/v2.13.0...v2.13.1)
View
43 features/command_line/require_option.feature
@@ -0,0 +1,43 @@
+Feature: --require option
+
+ Use the `--require` (or `-r`) option to specify a file to require
+ before running specs.
+
+ Scenario: using the --require option
+ Given a file named "logging_formatter.rb" with:
+ """ruby
+ require "rspec/core/formatters/base_text_formatter"
+ require 'delegate'
+
+ class LoggingFormatter < RSpec::Core::Formatters::BaseTextFormatter
+ def initialize(output)
+ super LoggingIO.new(output)
+ end
+
+ class LoggingIO < SimpleDelegator
+ def initialize(output)
+ @file = File.new('rspec.log', 'w')
+ super
+ end
+
+ def puts(message)
+ [@file, __getobj__].each { |out| out.puts message }
+ end
+
+ def close
+ @file.close
+ end
+ end
+ end
+ """
+ And a file named "spec/example_spec.rb" with:
+ """ruby
+ describe "an embarassing situation" do
+ it "happens to everyone" do
+ end
+ end
+ """
+ When I run `rspec --require ./logging_formatter.rb --format LoggingFormatter`
+ Then the output should contain "1 example, 0 failures"
+ And the file "rspec.log" should contain "1 example, 0 failures"
+ And the exit status should be 0
View
14 lib/rspec/core/configuration.rb
@@ -108,6 +108,9 @@ def self.add_setting(name, opts={})
# load order for files, declaration order for groups and examples).
define_reader :order
+ # Indicates files configured to be required
+ define_reader :requires
+
# Default: `$stdout`.
# Also known as `output` and `out`
add_setting :output_stream, :alias_with => [:output, :out]
@@ -200,6 +203,7 @@ def initialize
@fixed_color = :blue
@detail_color = :cyan
@profile_examples = false
+ @requires = []
end
# @private
@@ -492,7 +496,10 @@ def libs=(libs)
end
def requires=(paths)
+ RSpec.deprecate("RSpec::Core::Configuration#requires=(paths)",
+ "paths.each {|path| require path}")
paths.map {|path| require path}
+ @requires += paths
end
def debug=(bool)
@@ -827,9 +834,11 @@ def safe_include(mod, host)
end
# @private
- def add_project_paths(*paths)
- directories = paths.select { |p| File.directory? p }
+ def setup_load_path_and_require(paths)
+ directories = ['lib', default_path].select { |p| File.directory? p }
RSpec::Core::RubyProject.add_to_load_path(*directories)
+ paths.each {|path| require path}
+ @requires += paths
end
# @private
@@ -857,7 +866,6 @@ def configure_expectation_framework
# @private
def load_spec_files
- add_project_paths 'lib', default_path
files_to_run.uniq.each {|f| load File.expand_path(f) }
raise_if_rspec_1_is_loaded
end
View
23 lib/rspec/core/configuration_options.rb
@@ -19,15 +19,12 @@ def initialize(args)
end
def configure(config)
- formatters = options.delete(:formatters)
-
config.filter_manager = filter_manager
+ process_options_into config
- order(options.keys, :libs, :requires, :default_path, :pattern).each do |key|
- force?(key) ? config.force(key => options[key]) : config.send("#{key}=", options[key])
- end
+ config.setup_load_path_and_require(options[:requires] || [])
- formatters.each {|pair| config.add_formatter(*pair) } if formatters
+ load_formatters_into config
end
def parse_options
@@ -55,6 +52,8 @@ def filter_manager
MERGED_OPTIONS = [:requires, :libs].to_set
+ UNPROCESSABLE_OPTIONS = [:formatters, :requires].to_set
+
def force?(key)
!NON_FORCED_OPTIONS.include?(key)
end
@@ -66,6 +65,18 @@ def order(keys, *ordered)
keys
end
+ def process_options_into(config)
+ opts = options.reject { |k, _| UNPROCESSABLE_OPTIONS.include? k }
+
+ order(opts.keys, :libs, :default_path, :pattern).each do |key|
+ force?(key) ? config.force(key => opts[key]) : config.send("#{key}=", opts[key])
+ end
+ end
+
+ def load_formatters_into(config)
+ options[:formatters].each { |pair| config.add_formatter(*pair) } if options[:formatters]
+ end
+
def extract_filters_from(*configs)
configs.compact.each do |config|
filter_manager.include config.delete(:inclusion_filter) if config.has_key?(:inclusion_filter)
View
9 spec/rspec/core/configuration_options_spec.rb
@@ -25,14 +25,14 @@
opts = config_options_object(*%w[--require a/path -I a/lib])
config = double("config").as_null_object
config.should_receive(:libs=).ordered
- config.should_receive(:requires=).ordered
+ config.should_receive(:setup_load_path_and_require).ordered
opts.configure(config)
end
- it "sends requires before formatter" do
+ it "sets up load path and requires before formatter" do
opts = config_options_object(*%w[--require a/path -f a/formatter])
config = double("config").as_null_object
- config.should_receive(:requires=).ordered
+ config.should_receive(:setup_load_path_and_require).ordered
config.should_receive(:add_formatter).ordered
opts.configure(config)
end
@@ -105,7 +105,8 @@
with_env_vars 'SPEC_OPTS' => "--require file_from_env" do
opts = config_options_object(*%w[--require file_from_opts])
config = RSpec::Core::Configuration.new
- config.should_receive(:requires=).with(["file_from_opts", "file_from_env"])
+ config.should_receive(:require).with("file_from_opts")
+ config.should_receive(:require).with("file_from_env")
opts.configure(config)
end
end
View
29 spec/rspec/core/configuration_spec.rb
@@ -16,7 +16,7 @@ module RSpec::Core
end
end
- describe "#load_spec_files" do
+ describe "#setup_load_path_and_require" do
include_context "isolate load path mutation"
def absolute_path_to(dir)
@@ -28,7 +28,7 @@ def absolute_path_to(dir)
$LOAD_PATH.delete(lib_dir)
expect($LOAD_PATH).not_to include(lib_dir)
- config.load_spec_files
+ config.setup_load_path_and_require []
expect($LOAD_PATH).to include(lib_dir)
end
@@ -37,18 +37,26 @@ def absolute_path_to(dir)
foo_dir = absolute_path_to("features")
expect($LOAD_PATH).not_to include(foo_dir)
- config.load_spec_files
+ config.setup_load_path_and_require []
expect($LOAD_PATH).to include(foo_dir)
end
+ it 'stores the required files' do
+ config.should_receive(:require).with('a/path')
+ config.setup_load_path_and_require ['a/path']
+ expect(config.requires).to eq ['a/path']
+ end
+
context "when `default_path` refers to a file rather than a directory" do
it 'does not add it to the load path' do
config.default_path = 'Rakefile'
- config.load_spec_files
+ config.setup_load_path_and_require []
expect($LOAD_PATH).not_to include(match /Rakefile/)
end
end
+ end
+ describe "#load_spec_files" do
it "loads files using load" do
config.files_to_run = ["foo.bar", "blah_spec.rb"]
config.should_receive(:load).twice
@@ -1067,6 +1075,8 @@ def metadata_hash(*args)
end
describe "#libs=" do
+ include_context "isolate load path mutation"
+
it "adds directories to the LOAD_PATH" do
$LOAD_PATH.should_receive(:unshift).with("a/dir")
config.libs = ["a/dir"]
@@ -1074,9 +1084,18 @@ def metadata_hash(*args)
end
describe "#requires=" do
- it "requires paths" do
+ before { RSpec.should_receive :deprecate }
+
+ it "requires the configured files" do
+ config.should_receive(:require).with('foo').ordered
+ config.should_receive(:require).with('bar').ordered
+ config.requires = ['foo', 'bar']
+ end
+
+ it "stores require paths" do
config.should_receive(:require).with("a/path")
config.requires = ["a/path"]
+ expect(config.requires).to eq(['a/path'])
end
end
Something went wrong with that request. Please try again.