Require users to invoke `RSpec.reset` #703

Closed
wants to merge 2 commits into from

5 participants

@samphippen
RSpec member

This is related to #621.

In the issue report it seems to have been decided that the best course of action is to require users to invoke RSpec.reset instead of it being called automatically. This pull request does that.

@samphippen samphippen Remove the automatic call to RSpec.reset
This also updates the specs that are effected by this call. The order
specs were effected because the RSpec runner was being invoked within
the same ruby process (I think). The run_command helper in the
order_spec file has been updated to explicitly invoke the reset method.

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
d6d0d35
@samphippen
RSpec member

@dchelimsky you were pretty involved with 621. Can you take a look at this please? Cheers :)

@pierreozoux pierreozoux referenced this pull request in sandro/specjour Oct 15, 2012
Closed

Rspec-core current dev > 2.11.1 #44

@wallace

@myronmarston @alindeman , I know @dchelimsky is lessening his work in rspec. Can someone else review and merge, please? This will close #621.

@dchelimsky
RSpec member

FWIW I think this is a potentially breaking change that would need to be released as part of 3.0. It also should be documented very clearly, which it is not yet.

@wallace

@dchelimsky, good point.

@samphippen are you interested in updating this PR with documentation? If not, I can take a stab at.

@JonRowe
RSpec member

@myronmarston should this not be tagged for rspec 3?

@samphippen
RSpec member

I somehow missed these emails. I'll take a look at this in the near future.

@JonRowe
RSpec member

This is also a dependency for #621 which is similarly awaiting RSpec 3

@samphippen samphippen Add some docs for the new reset behaviour
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
4fe3704
@samphippen
RSpec member

I added some docs.

@mskraddoc

I am seeing a 'stack level too deep' error immediately following a call to rspec.reset as detailed in a lengthy SO question here: http://stackoverflow.com/questions/16512114/stack-level-too-deep-systemstackerror-when-using-both-rspec-and-cucumber-with

Could this be related?

@JonRowe
RSpec member

I'm afraid I don't believe these are related, StackLevelTooDeep errors are caused by your Ruby interpreter running out of stack, normally they are caused by infinite recursion but sometimes they can be caused by long running loops, looking at your SO question I'm hazarding a guess that the rspec call is incidental, especially as it the ActiveSupport dependencies code where the exception is raised. If you can piece together a reproducible example as a git repo I might be able to take a look at it for you, but so far on SO you've not posted any application code which is most likely where the culprit lies.

@samphippen
RSpec member

If I rebase this, now that we're working towards 3.0 is this something that we want merged. @myronmarston @JonRowe @soulcutter @alindeman @dchelimsky

@JonRowe
RSpec member

Pass, have we figured out if it's definitely needed? What about docs? How would this work in practise?

@samphippen
RSpec member

@dchelimsky @myronmarston I don't know what's going on with this, but don't want to keep this open for too long, what should we do about it?

@dchelimsky
RSpec member

I'm concerned by the implications of #621 (comment). @sandro's issue was that clearing the config between runs blew away config that wasn't going to get reloaded. Right now we have no concept of config settings that should be persistent vs those that should be clearable. I'm not even sure we can do that in a universally correct way. I'll ponder this a bit and follow up if anything hits me, but I don't recommend merging this yet as it solves one problem by creating another IMO.

@samphippen
RSpec member

I think I'm going to close this unless we think of a better solution, based on what @dchelimsky says.

@samphippen samphippen closed this Aug 26, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment