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.

Sign up for GitHub

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

Jump to bottom

Ensure all tests run individually #267

Merged
merged 3 commits into from Oct 18, 2016
Merged

Ensure all tests run individually #267

merged 3 commits into from Oct 18, 2016

Conversation

chrisroos
Copy link
Member

@chrisroos chrisroos commented Sep 29, 2016 •
edited

I'd noticed that running some tests individually (i.e. outside of rake test) would result in failures.

I created a script to run each test file individually which allowed me to find and fix problems in the following two tests:

  • test/unit/any_instance_test.rb
  • test/unit/class_method_test.rb

I believe this fixes #251.

@chrisroos
Copy link
Member Author

@floehopper: Can you take a look at this when you have a moment please?

@floehopper
Copy link
Member

While I agree that the current behaviour is wrong and that your change fixes the problem, I'm not sure it's the right way to fix the problem. Requiring 'mocha/object_methods' only fixes the problem very indirectly - it only defines the ObjectMethods module - it's only because of the inclusion of 'test/method_definer' that this module is included into Object.

Requiring 'mocha/object_methods' also has the effect of requiring a load of other files. Given that this is intended to be a unit test, I don't think that's very desirable.

I suspect what's happened is that this test was originally "stubbing" (using 'test/method_definer' methods) various bits of ClassMethod in order to avoid dependencies on other files/classes/modules. As the implementation of ClassMethod has changed, the unit test hasn't kept up and is missing some "stubs".

Looking through the code, I think the re-opening of the Mocha::ObjectMethods module and its inclusion into Object is in a bit of a muddle. I've created a new issue, #268, to handle this.

@chrisroos
Copy link
Member Author

Thanks, @floehopper. I did wonder whether this was the correct solution.

I'll investigate stubbing the required methods instead of including Mocha::ObjectMethods.

@chrisroos
Copy link
Member Author

I've updated this branch so that I'm stubbing methods on ClassMethod, AnyInstanceMethod and the stubbee objects created in the tests. The commits contain a couple of different approaches as I'm not completely sure which way to go.

@floehopper: Do you have thoughts about:

  1. Should I stub ClassMethod#reset_mocha or stubbee.reset_mocha in class_method_test and any_instance_method_test?
  2. Should I use define_instance_method(:_method) or __metaclass__.send(:alias_method, :_method) in class_method_test?

@floehopper
Copy link
Member

@chrisroos:

  1. Should I stub ClassMethod#reset_mocha or stubbee.reset_mocha in class_method_test and any_instance_method_test?

I think I would stub ClassMethod#reset_mocha unless an individual test is explicitly checking that stubbee.reset_mocha is being called; I'm not sure any of them currently are.

  1. Should I use define_instance_method(:_method) or __metaclass__.send(:alias_method, :_method) in class_method_test?

I don't feel strongly either way. If I had to choose, I'd probably use __metaclass__.send(:alias_method, :_method), because it's closer to the implementation in ObjectMethods.

As I've previously mentioned, I'm not very happy with these tests. However, I'm happy that your changes don't make them any worse and it'd be great to have them passing when run on their own.

@chrisroos
Fix failing tests in any_instance_method_test
55417f9 
I was seeing the following 2 failures when running the tests in this
test case:

1. AnyInstanceMethodTest#test_should_call_remove_new_method:
   Mocha::ExpectationError: unexpected invocation:
   #<Mock:0x7fe5d756b958>.reset_mocha()

2. AnyInstanceMethodTest#test_should_call_restore_original_method:
   Mocha::ExpectationError: unexpected invocation:
   #<Mock:0x7fe5d756b840>.reset_mocha()

These tests would pass when run using `rake test` because
`Mocha::ObjectMethods` was being included elsewhere which meant that the
real implementation of `reset_mocha` was being invoked.

NOTE. These tests were previously failing in Ruby 1.8.7, 1.9.3, 2.0.0,
2.1, 2.2 and 2.3.
@chrisroos
Fix failing tests in class_method_test
14982c2 
I was seeing the following 2 failures when running the tests in this
test case:

1. ClassMethodTest#test_should_call_restore_original_method:
   NoMethodError: undefined method `reset_mocha' for #<Class:0x007fc4374b0728>

2. ClassMethodTest#test_should_call_remove_new_method:
   NoMethodError: undefined method `reset_mocha' for #<Class:0x007fc4374aaaa8>

These tests would pass when run using `rake test` because
`Mocha::ObjectMethods` was being included elsewhere which meant that the
real implementation of `reset_mocha` was being invoked.

NOTE. These tests were previously failing in Ruby 1.8.7, 1.9.3, 2.0.0,
2.1, 2.2 and 2.3.
@chrisroos
Fix failures in class_method_test under 1.8.7/1.9.3
bfd1b34 
I was seeing the following 3 failures when running the tests in this
test case:

1. test_should_hide_original_method(ClassMethodTest) [test/unit/class_method_test.rb:18]:
   <false> expected but was
   <true>.

2. test_should_restore_original_method(ClassMethodTest):
   NameError: undefined method `method_x' for class `Class'

3. test_should_restore_original_method_accepting_a_block_parameter(ClassMethodTest):
   NameError: undefined method `method_x' for class `Class'

Each of these failures was caused by the absence of the `#_method`
method. Invoking this method in `#hide_original_method` was raising an
exception that was being caught by the `rescue NameError` block. I've
used `alias_method` to define the `#_method` method as this matches the
real behaviour in `Mocha::ObjectMethods`.

These tests would pass when run using `rake test` because
`Mocha::ObjectMethods` was being included elsewhere which meant that the
real implementation of `_method` was being invoked.

NOTE. These tests were previously failing in Ruby 1.8.7 and 1.9.3.
Versions 2.0.0 and above use the prepended modules approach which don't
involve the use of `_method`.
@chrisroos chrisroos force-pushed the ensure-all-tests-run-individually branch from 0d86d68 to bfd1b34 Compare October 18, 2016 13:32
@chrisroos
Copy link
Member Author

Thanks for the feedback, @floehopper. I've now rewritten the commits in this branch and plan to merge if/when the tests have all passed.

@chrisroos chrisroos merged commit ccbdbc9 into master Oct 18, 2016
@chrisroos chrisroos deleted the ensure-all-tests-run-individually branch October 18, 2016 15:37
This was referenced Mar 21, 2018
Closed
Closed
Closed
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Some tests in ClassMethodTest fail when it is run on its own
2 participants
@chrisroos @floehopper