Prevent use of Mocha outside the context of a test/example #327

floehopper · 2018-03-30T12:08:55Z

This change means that any attempt to use Mocha outside the context of a test or example will result in a Mocha::NotInitializedError being raised with the following message: "Mocha methods cannot be used outside the context of a test".

Fixes #292
Fixes #293 (indirectly)
Supersedes #319

Both Mocha::Integration::MiniTest::Adapter and Mocha::Integration::TestUnit::Adapter were already calling Mocha::Hooks#mocha_setup, but the legacy integration modules were not. Even though Mocha::Hooks#mocha_setup is currently a no-op, I'm planning to change this and making this change now means I'm starting from a consistent baseline. Note that I've confirmed that RSpec::Core::MockingAdapters::Mocha#setup_mocks_for_rspec already calls Mocha::Hooks#mocha_setup [1]. [1]: https://github.com/rspec/rspec-core/blob/a8aae27114dde9ebc429a168b9b3c63dc18e5088/lib/rspec/core/mocking_adapters/mocha.rb#L44

lzap · 2018-04-03T12:15:45Z

lib/mocha/mockery.rb

    class << self

      def instance
-        @instance ||= new
+        @instances.last || Null.new


This throws undefined method "last" for nil:NilClass when you attempt to use mocha uninitialized (e.g. example snippets from documentation). This dirty change fixes it:

@instances.nil? ? Null.new : @instances.last

@lzap Gah! Looks like I went wrong with some of my interactive rebasing. Thanks for testing - I'll try to sort it out as soon as I can.

@lzap I'm in the process of fixing this and just to warn you I plan to force-push the branch shortly.

lzap · 2018-04-03T12:24:20Z

Works great here, I vote for some easy opt-out, I can imagine this can break tests for some. An environmental variable should be good enough for those who don't want to fix their tests, can't or want to postpone this. Kudos, great job!

Previously the Mockery singleton instance was lazily instantiated the first time it was needed. This meant that it could be instantiated outside the scope of a test with no subsequent calls to Mockery.verify, Mockery.teardown, or Mockery.reset_instance. This had the potential to leave things in an inconsistent state and resulted in confusing errors in tests. See #292 for some examples reported by users. This change is a step on the way to solving that issue. Note that rather than simply instantiating a single instance of a Mockery, I've had to store a stack of instances. This is entirely to cope with the acceptance tests which result in nested calls to Mockery.setup and Mockery.teardown: 1. Outer acceptance test: Mockery.setup 2. Inner test in run_as_test(s) block: Mockery.setup 3. Inner test in run_as_test(s) block: Mockery.teardown 4. Outer acceptance test: Mockery.teardown I've also had to copy the logger instance from the outer Mockery instance to the inner one in order that the outer acceptance tests have access to the same logger as used by the inner one. Obviously the above complication is a bit undesirable, given that it's only needed to support the acceptance tests and should never be required by users. However, I tried various alternatives and I felt this one was the best option. Note also that this indirectly addresses #293.

Now that the Mockery instance is being explicitly instantiated and destroyed, I think this restoration of the original logger is redundant.

Hopefully this addresses #292. If no Mockery has been instantiated when a call is made to any method which would otherwise change the state of the Mockery instance then raise a NotInitializedError. This is achieved by returning an instance of Mockery::Null if no Mockery has been instantiated. Note that the Mockery::Null class inherits from the Mockery class, but overrides all methods which result in a change of state.

floehopper · 2018-04-06T15:15:07Z

@lzap:

Works great here

Thanks for testing it. 👍

I vote for some easy opt-out, I can imagine this can break tests for some. An environmental variable should be good enough for those who don't want to fix their tests, can't or want to postpone this.

I'm hesitant to offer an opt-out, because Mocha was never intended to be used outside the context of a test. I'm probably going to release it like this and see whether anyone runs into problems.

In any case, a better solution for that problem is probably to call Mocha::Hooks#mocha_setup from an appropriate place and at least consider also calling Mocha::Hooks#mocha_verify and Mocha::Hooks#mocha_teardown which is what the various test frameworks use. See this documentation for more info.

floehopper · 2018-04-06T16:18:54Z

I just ran the tests in the GOV.UK Whitehall project with these changes and saw no ill effects, so I'm happy to merge.

floehopper · 2018-04-06T16:41:06Z

@lazyatom has just updated Kintama to work with this change and all appears to be well.

floehopper · 2018-04-06T17:04:09Z

I've released this change in v1.5.0.

Evan-M · 2018-04-06T22:02:28Z

I'm probably going to release it like this and see whether anyone runs into problems.

🙋‍♂️ I'm running into problems... A gem I've been contributing to now has failing tests on Travis CI for all branches. One of the tests stubs out method calls in before blocks of a handful of controller tests/examples; while ultimately the test in question should probably be refactored, I would not have expected an updated dependency to break a bunch of tests either. (To be fair, the Gemfile does not specify a version of Mocha, and Travis CI seems to have updated to 1.5.0 automagically, which I also didn't expect)

(A release with a deprecation warning instead of raising an exception might have been nice in this case).

floehopper · 2018-04-06T22:26:10Z

@Evan-M Sorry to hear that. Can you point me at one of the tests which is failing? What test framework are you using? There shouldn't be a problem stubbing methods in a before block, so that's a bit confusing.

floehopper · 2018-04-06T23:27:40Z

@Evan-M I've left a comment on the issue you raised on lynndylanhurley/devise_token_auth. I hope it helps. Let me know if you have any questions.

Evan-M · 2018-04-06T23:29:25Z

Yeah, no problem... I should have linked to the failures in my original comment.

A little more pretext:
The first encounter with the problem was in the build status for Travis after merging lynndylanhurley/devise_token_auth#1134. Everything passed on the PR, and then started failing midway through the CI build jobs for master after merging the PR. The job logs for the build report a failing test suite, with the error:

Mocha::NotInitializedError: Mocha methods cannot be used outside the context of a test

When I tried to re-run the build, all the build failed; I suspect I merged lynndylanhurley/devise_token_auth#1134 right about the same time that version 1.5.0 was released.

The same build failure / test failure appears to happen for another (unmerged) PR as well.

What test framework are you using?

The test suite is based on Minitest, but there is a mixture of the Minitest Spec DSL and the Minitest Test DSL. (It was that way when I became involved in the project 🤷‍♂️).

There shouldn't be a problem stubbing methods in a before block, so that's a bit confusing.

Yeah, I was wondering about stubbing methods in a before block (as opposed to a before(:all) block, which doesn't even exist in Minitest!).

Can you point me at one of the tests which is failing?

Anyway, the repo is lynndylanhurley/devise_token_auth. The failing tests are all in the test file /test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb:

https://github.com/lynndylanhurley/devise_token_auth/blob/7df89606ef127961c1957aaccf714ff322ebf431/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb#L72-L75

https://github.com/lynndylanhurley/devise_token_auth/blob/7df89606ef127961c1957aaccf714ff322ebf431/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb#L147-L166

https://github.com/lynndylanhurley/devise_token_auth/blob/7df89606ef127961c1957aaccf714ff322ebf431/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb#L168-L182

Also the failing tests in question here are actually part of an Integration Test, so it is probably a bad idea to stub any method calls!

floehopper · 2018-04-06T23:41:05Z

@Evan-M The upgrade to Mocha v1.5.0 definitely revealed a problem, but I think there was already an underlying problem in that test. Did you see my comment: lynndylanhurley/devise_token_auth#1137 (comment) ? If you fix that, there's shouldn't be a problem stubbing/expecting methods in a before block and using Mocha v1.5.0.

floehopper added feature-request work-in-progress labels Mar 30, 2018

This was referenced Mar 30, 2018

[WIP] Fail fast if mocking before setup #319

Closed

Prevent calls to Mocha::API methods until the before/setup block for a single test/example #292

Closed

lzap reviewed Apr 3, 2018

View reviewed changes

floehopper added 5 commits April 6, 2018 15:15

Remove unnecessary restoration of Mockery.logger in acceptance tests

8ea0c63

Now that the Mockery instance is being explicitly instantiated and destroyed, I think this restoration of the original logger is redundant.

Extract ErrorWithFilteredBacktrace to reduce duplication

b57c386

Inline Mockery.reset_instance into Mockery.teardown

89eeb1c

floehopper force-pushed the prevent-use-of-mocha-outside-test branch from e89f7fc to 89eeb1c Compare April 6, 2018 14:37

floehopper merged commit d6c6c82 into master Apr 6, 2018

floehopper deleted the prevent-use-of-mocha-outside-test branch April 6, 2018 16:19

treydock mentioned this pull request Apr 14, 2018

add sensu version facter (continued) sensu/sensu-puppet#894

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent use of Mocha outside the context of a test/example #327

Prevent use of Mocha outside the context of a test/example #327

floehopper commented Mar 30, 2018 •

edited

lzap Apr 3, 2018 •

edited

floehopper Apr 4, 2018

floehopper Apr 6, 2018

lzap commented Apr 3, 2018

floehopper commented Apr 6, 2018

floehopper commented Apr 6, 2018

floehopper commented Apr 6, 2018

floehopper commented Apr 6, 2018

Evan-M commented Apr 6, 2018 •

edited

floehopper commented Apr 6, 2018

floehopper commented Apr 6, 2018

Evan-M commented Apr 6, 2018

floehopper commented Apr 6, 2018

Prevent use of Mocha outside the context of a test/example #327

Prevent use of Mocha outside the context of a test/example #327

Conversation

floehopper commented Mar 30, 2018 • edited

lzap Apr 3, 2018 • edited

Choose a reason for hiding this comment

floehopper Apr 4, 2018

Choose a reason for hiding this comment

floehopper Apr 6, 2018

Choose a reason for hiding this comment

lzap commented Apr 3, 2018

floehopper commented Apr 6, 2018

floehopper commented Apr 6, 2018

floehopper commented Apr 6, 2018

floehopper commented Apr 6, 2018

Evan-M commented Apr 6, 2018 • edited

floehopper commented Apr 6, 2018

floehopper commented Apr 6, 2018

Evan-M commented Apr 6, 2018

floehopper commented Apr 6, 2018

floehopper commented Mar 30, 2018 •

edited

lzap Apr 3, 2018 •

edited

Evan-M commented Apr 6, 2018 •

edited