Skip to content

Fix for #1280 #1289

Merged
merged 3 commits into from Feb 6, 2014

3 participants

@myronmarston
RSpec member

This is the start of a fix for #1280. So far I've only gotten as far as a test exposing the issue.

The "easy" fix for this is to reorder things so that the files_to_run are resolved before requires are processed. However, that would break the fix @JonRowe put in place in 25d6b3a, which allows pattern to be set in an RSpec.configure block if you use a CLI option to require your spec helper. pattern can affect files_to_run so the change he made makes sense. I'm not sure what the best fix is. A few ideas:

  • Change things so that files_to_run, rather than being assigned at a particular point in time, is lazily computed on first access. If the user sets pattern before accessing files_to_run it would take the pattern into account but otherwise it wouldn't. This might require a large amount of refactoring.
  • Provide a separate predicate method like config.running_one_spec_file? that is set to true if and only if a single file name has been passed at the command line. I think this can be computed apart from the pattern so it could happen earlier.

@JonRowe, what are your thoughts, since you've worked in this area before?

I'm interested in everyone's feedback, though.

/cc @xaviershay @alindeman @soulcutter @samphippen

@JonRowe
RSpec member
JonRowe commented Feb 5, 2014

I think I'd prefer they be lazily compiled, to give people the most chance to set things like pattern, but it will lead to some confusing behaviour (e.g. if you check theres only one file before setting pattern)

myronmarston added some commits Feb 5, 2014
@myronmarston myronmarston Refactor: remove special cases from configuration options.
It's easiest to leverage the same "order then set configs"
logic of `process_options_into` rather than treating
libs and requires as special.
feead38
@myronmarston myronmarston Rename `parse_options` to `options` and have it return a memoized value.
The old way was odd. It created an ordering dependency
(you had to call `parse_options` after instantiating the
object but before doing anything else) and calling it a second
time had the side effect of re-parsing the args, which wasn't
needed or intended.

It's far easier/better to make `options` do the parsing
and memoize the result.
b2fea38
@myronmarston myronmarston Allow `files_to_run` to be used in `spec_helper` when loaded early vi…
…a `-r`.

This changes it to be lazily rather than eagerly computed.
This is important to allow `pattern` to be set from a `spec_helper`
file loaded via `-r`.
b342ffb
@myronmarston
RSpec member

I went with the lazy solution. Can you review this, @JonRowe?

@JonRowe JonRowe commented on the diff Feb 6, 2014
features/command_line/init.feature
@@ -22,14 +22,36 @@ Feature: --init option
Given I have a brand new project with no files
And I have run `rspec --init`
When I accept the recommended settings by removing `=begin` and `=end` from `spec/spec_helper.rb`
- And I create a spec file with the following content:
- """
+ And I create "spec/addition_spec.rb" with the following content:
+ """ruby
@JonRowe
RSpec member
JonRowe added a note Feb 6, 2014

An aside, is this for relish?

@myronmarston
RSpec member
myronmarston added a note Feb 6, 2014

You mean the """ruby bit? Yes, that's for relish -- it tells it what kind of text it is, so it can apply syntax highlighting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JonRowe JonRowe commented on the diff Feb 6, 2014
lib/rspec/core/configuration.rb
@@ -897,7 +902,7 @@ def safe_include(mod, host)
end
# @private
- def setup_load_path_and_require(paths)
+ def requires=(paths)
@JonRowe
RSpec member
JonRowe added a note Feb 6, 2014

why the name change? it's still manipulating the load path....

@myronmarston
RSpec member
myronmarston added a note Feb 6, 2014

Because in feead38 I did some prep refactoring that removed calling setup_load_path_and_require as a special case. Now it treats the requires option just like any other option, and calls the corresponding writer method on configuration to set it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JonRowe
RSpec member
JonRowe commented Feb 6, 2014

Looks good to me, and surprisingly simple...

@myronmarston myronmarston merged commit c8a45e9 into master Feb 6, 2014

1 check passed

Details default The Travis CI build passed
@myronmarston myronmarston deleted the fix-spec-helper-files-to-run branch Feb 6, 2014
@vshvedov

Wasn't it possible to deprecate it first, at least for beta period?

RSpec member

Wasn't it possible to deprecate it first, at least for beta period?

We could add a deprecation to 2.99, but the entire RSpec::Core::ConfigurationOptions class is a private API (notice the # @private declaration above it). If we had to add a deprecation for every internal refactoring that changed a private API we would make virtually no progress towards releasing 3.0.

Did this cause a problem for you? In the future, please rely only on RSpec APIs that are documented as being public (which is different from public visibility in ruby). Private APIs are subject to change at any time in any patch, minor or major release. If there's something you need and there's no public API for it, open an issue, and the vast majority of the time we'll add one for you and ensure it gets documented as being public, at which point it is subject to SemVer.

Yep, indirectly. guard-rspec is broken this way, but it was not with beta1.

Thanks for a detailed response, I'll figure out the cause of

15:17:24 - INFO - Run all
15:17:24 - INFO - Running all specs
15:17:24 - ERROR - Guard::RSpec failed to achieve its <run_all>, exception was:
> [#B37FC1600F05] NoMethodError: undefined method `parse_options' for #<RSpec::Core::ConfigurationOptions:0x00000102a0fbd8 @args=[]>
RSpec member

If guard-rspec needs something not provided by any of RSpec's currently documented public APIs let us know.

@yonkeltron yonkeltron added a commit to SexualHealthInnovations/stck-clinic that referenced this pull request Feb 26, 2014
@yonkeltron yonkeltron added guard even though broken: rspec/rspec-core#1289 5a5f5e8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.