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

Deprecate passing block to mock object constructor #290

Merged
merged 2 commits into from Jan 11, 2017

Conversation

floehopper
Copy link
Member

This mechanism is confusing, because the block is evaluated in the
context of the newly instantiated block and not in the context of the
current test.

The mechanism is also of dubious value, because it's possible to use
Object#tap or define stubs/expectations with the mock object as the
explicit receiver without the code becoming much more verbose.

It's unfortunate that the deprecations make the tests more complicated,
but we will be able to simplify them again when this functionality is
removed in a later version.

See #286 for further details.

@floehopper
Copy link
Member Author

@chrisroos: I'd be grateful if you could review this at some point.

@floehopper floehopper force-pushed the deprecate-passing-block-to-mock-object-constructor branch from 706286d to 5925e04 Compare December 5, 2016 10:45
@floehopper
Copy link
Member Author

I've rebased against master and force-pushed in the hope of fixing the Travis CI build for this branch.

@floehopper
Copy link
Member Author

Build is now fixed.

This is to address the confusion experienced by @ggaston095 in #286.

I've chosen to use two "@yield" lines in the docs, because
YARD converts these into separate bullet points under the "Yields:"
heading which seemed better than a single bullet point with a very
long line of text.
This mechanism is confusing, because the block is evaluated in the
context of the newly instantiated block and not in the context of the
current test.

The mechanism is also of dubious value, because it's possible to use
Object#tap or define stubs/expectations with the mock object as the
explicit receiver without the code becoming much more verbose.

It's unfortunate that the deprecations make the tests more complicated,
but we will be able to simplify them again when this functionality is
removed in a later version.

See #286 for further details.
@floehopper floehopper force-pushed the deprecate-passing-block-to-mock-object-constructor branch from 5925e04 to 9316807 Compare December 14, 2016 11:46
@floehopper
Copy link
Member Author

I've now introduced an earlier commit which improves the documentation of the existing behaviour, before that behaviour is then deprecated in the subsequent commit.

@chrisroos: It'd be great if you could have a look at this when you have a chance. I don't think it should take you long. Let me know if you want help on how to generate the documentation...

@chrisroos
Copy link
Member

Out of interest, why have you used DeprecationDisabler.disable_deprecations (i.e. the module method) in mock_with_initializer_block_test.rb and disable_deprecations (i.e. the included instance method) in mockery_test.rb?

As an observation, I presume there aren't any tests for this now-deprecated behaviour for stub and stub_everything.

Aside from my question and observation this all looks good to me, @floehopper.

@floehopper
Copy link
Member Author

@chrisroos:

Thanks for reviewing.

Out of interest, why have you used DeprecationDisabler.disable_deprecations (i.e. the module method) in mock_with_initializer_block_test.rb and disable_deprecations (i.e. the included instance method) in mockery_test.rb?

In the former, each test is instantiating a new instance of a test case (via TestRunner#run_as_test) and the DeprecationDisabler` module would need to be included into each instance. So I decided it was simpler to just use the module method in this case. Does that make sense?

As an observation, I presume there aren't any tests for this now-deprecated behaviour for stub and stub_everything.

You are correct. I think I'm OK with this on the basis I know that the behaviour is implemented within Mock#initialize, i.e. I'm using a pragmatic white-box testing approach.

I'm planning to get this merged, so do shout if you have any more comments or questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants