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

Lifecycle/space fixups #519

Merged
merged 22 commits into from Jan 9, 2014
Merged

Lifecycle/space fixups #519

merged 22 commits into from Jan 9, 2014

Conversation

@myronmarston
Copy link
Member

myronmarston commented Jan 4, 2014

This is the start of addressing a bunch of issues.

  • Fixes #165 -- stop resetting mock proxies twice at the end of each example
  • Fixes #352 -- make test doubles self-destruct after they are torn down.
  • Fixes #240 -- adds with_temporary_scope.

Ready for review.

myronmarston added 10 commits Jan 3, 2014
The logic that required this test was removed in cd81094.
We don't need to reset the mock proxy while verifying it.
That'll happen later during the teardown phase.

It looks like the main reason we were doing this was
to assist with rspec-mocks testing itself; we would
verify the middle of an example to assert a mock expectation
failure, and a `reset` is needed so it doesn't fail again
when rspec-core calls `verify` as well.

The solution is to use spec helper methods that perform
the reset when we verify in the middle of an example.

I don't have any evidence this improves perf, but it
seems reasonable to assume that removing an extra method
call per example can only make it faster (however slight
the improvement may be).

Fixes #165.
Test doubles are not designed to be used outside of
the example they were created in.
We're going to change the behavior of test doubles
that have been reset (so that they "expire" and can
no longer be used). This spec was originally added in
847c66d where the
point was to address an error while resetting a test
double (which, in turn caused that method double
to stick around into later examples and continue to
fail).
The second part of this spec was added in
9357e4d. We are
changing test doubles so that they self-destruct
after being reset. Currently, `verify` also
resets, so verifying a test double twice
will no longer be allowed.
We're going to change how pure test doubles behave
after teardown, so update this to focus on partial
doubles (which will not be changed).
Also, document some of them as public since
users may want to rescue them, or specify
a block will raise one of these errors
(e.g. in an rspec-mocks extension gem).
There's no need for a separate file and there's
going to be a bit of interaction between multiple
spaces at various layers in a stack so having it
in one file will make it easier to see how it relates.
* This should be faster (but almost certainly not
  noticably so).
* It was odd to expose these methods off of `RSpec::Mocks`
  given that they are not intended for usage outside
  rspec-mocks.
* We weren't getting any benefit from these except
  for not having to type `.space`.
@penelopezone
Copy link
Member

penelopezone commented Jan 4, 2014

@myronmarston I took a look through this. Looks good :)

myronmarston added 11 commits Jan 4, 2014
This unifies global storage of modified things in one place.
- Make it more general (it's not just about stubbing).
- Remove unused lines.
- Improve doc output (`include_examples` does not
  create a nested group).
When rspec-expectations tries to diff the object in
a failure message, it blows up when the `pp` library
tries to print it.
Closes #240.
- It was duplicated in Proxy.
- Makes more sense to initialize it in Space init.
rspec-mocks lacks the necessary sandboxing to safely
define and run examples from within examples.
rspec-core has and uses this but it's not exposed
for use here.

Instead, we can just trigger the teardown/setup
that happens between examples.

This fixes some rspec-mocks space leakage
that was happening.
Not sure how we wound up with two separate files, anyway.
Since we now use a new space instance per example,
we don't need to clear its collections when resetting.
Before this was necessary because we kept a space instance
that we would keep using for the lifetime of the process.

Constant mutators weren't being reset idempotently, so I
had to tweak them a bit.
@@ -37,7 +25,8 @@ def self.verify
# each example, even if an error was raised during the example.
def self.teardown
space.reset_all
self.space = ERROR_SPACE
@space_stack.pop

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jan 7, 2014

Member

You could combine these two lines to @space = @space_stack.pop || @root_space no?

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jan 7, 2014

Author Member

Nope. @space should be set to @space_stack.last after popping.

This comment has been minimized.

Copy link
@JonRowe
# Performs per-test/example setup. This should be called before
# an test or example begins.
def self.setup
self.space = MOCK_SPACE
@space_stack << (@space = space.new_scope)

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jan 7, 2014

Member

Why don't we just access @space_stack.last everywhere?

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jan 7, 2014

Member

Or define space to be that?

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jan 7, 2014

Author Member

I initially planned on doing that, but then was noticing that we access RSpec::Mocks.space all over the place from the rest of the rspec-mocks code (e.g. to call proxy_for on it), and while Array#last shouldn't be a slow method call, my instinct is that it can only be faster to have space already available for direct access. The cost of doing this is only the extra memory need for the @space ivar, but who cares about that?

Anyhow, putting it in the ivar also allows us to use attr_reader, which is apparently faster than any normal method definition could be:

rails/arel@ea1d050

"another example and can no longer be used. rspec-mocks' doubles are " +
"designed to only last for one example, and you need to create a new " +
"one in each example you wish to use it for."
end

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jan 7, 2014

Member

Would this be raised for a double created in a temporary scope then reused? Could that be confusing?

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jan 7, 2014

Author Member

Yep. I tried to rephrase this to work for the temporary scope case but I couldn't come up with a phrasing I liked. On top of that, I'd consider using with_temporary_scope to be an "advanced" feature of rspec-mocks. I think a newcomer (who has no idea what "an rspec-mocks scope" is) is more likely to be confused by a message worded in those terms than someone using with_temporary_scope being confused by this message.

Anyhow, I'm open to alternate phrasings if you have an idea. I was also thinking it could be good to mention with_temporary_scope in the messages raised from RootSpace so people know that's available but I couldn't come up with a good way to include that in the message. Any ideas?

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jan 7, 2014

Member

How about passing a variable to this method that defaults to example but when raised from with_temporary_scope sets it to context, then it'd be slightly less confusing in that scenario?

"#{intro} was originally created in one example but has leaked into " +
"another context and can no longer be used. rspec-mocks' doubles are " +
"designed to only last for one context, and you need to create a new " +
"one in each context you wish to use it for."

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jan 9, 2014

Author Member

I've tried to apply your suggestion, but applying it gets really complicated for all the different possibilities:

  • Created in a temporary scope, used in a later example.
  • Created in a temporary scope, used in a later temporary scope.
  • Created in an example, used in a later example.
  • Created in an example, used in a temporary scope after the example.

Doing this properly will require a bunch of extra state, for what is an edge case. I'm not convinced it's worth it...but maybe you want to take a stab at it after this is merged?

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jan 9, 2014

Member

maybe ;)

super
end

private

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jan 7, 2014

Member

This should be dedented two spaces no?

@JonRowe
Copy link
Member

JonRowe commented Jan 7, 2014

Looking good so far but I left a few notes :)

This is our preferred convention.
myronmarston added a commit that referenced this pull request Jan 9, 2014
Lifecycle/space fixups
@myronmarston myronmarston merged commit 8615dc2 into master Jan 9, 2014
1 check passed
1 check passed
default The Travis CI build passed
Details
@myronmarston myronmarston deleted the lifecycle-space-fixups branch Jan 9, 2014
@myronmarston myronmarston mentioned this pull request Feb 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.