freerange / mocha

A mocking and stubbing library for Ruby
https://mocha.jamesmead.org
Other
1.2k stars 156 forks source link

A "targeted" version of #any_instance #653

Open brandoncc opened 1 week ago

brandoncc commented 1 week ago

I often need to assert that an object representing a specific database row (ActiveRecord instance, for example). This falls apart when the database query is made in a place outside the control of the test. Without mocking pieces of the system such as ActiveRecord queries, there isn't any way to access the instances and add expectations to them. The problem is, I don't always want to mock. For example, I'm currently writing a test that runs a background job and makes sure that only the correct objects receive a method call. If I mock the ActiveRecord call, that makes the whole test worthless.

Is there a way to do something like this?

MyClass.instance_with(id: 7).expects(:a_call)
MyClass.instance_with(id: 8).expects(:a_call).never

If there isn't, is this something that you would consider adding or accepting into the library?

floehopper commented 4 days ago

@brandoncc

Thanks for your interest in Mocha.

Is there a way to do something like this?

MyClass.instance_with(id: 7).expects(:a_call)
MyClass.instance_with(id: 8).expects(:a_call).never

Not specifically. I suppose you could stub MyClass.new or MyClass#initialize and return different mock objects depending on the arguments passed in, but I haven't tried it and I wouldn't recommend it.

If there isn't, is this something that you would consider adding or accepting into the library?

My initial reaction is probably not. In the past I've even contemplated disabling Mocha::ClassMethods#any_instance by default or even moving it into some kind of Mocha extension. This is because I regard the need to use it as an indicator of a tightly-coupled design. A TDD approach encourages changing the design to make it easier to test. However, I recognize that Mocha::ClassMethods#any_instance can be useful when trying to test "legacy" code or code that you do not control. Also the Rails framework often encourages quite tight coupling, as you have found with ActiveJob/ActiveRecord.

If I mock the ActiveRecord call, that makes the whole test worthless.

Can you expand on this a bit? Do you mean you don't want to stub the MyClass.find(id) call that's happening in your job?

brandoncc commented 4 days ago

Thanks for the response, and I agree that #any_instance is generally a pattern to be avoided.

I was writing an integration-style test for a background job, and wanted to test that only some objects were processed. "Processed" in this scenario means "receive a method call".

I know the preferred surface to test is the outcome of the #a_call, but I can't remember if there was a testable change on the object. If there wasn't, then setting an expectation that a method should/shouldn't be called on specific objects is another approach that could be used.


I agree with getting rid of #any_instance in principle, but I would encourage you to think about possibly replacing it with something like #instance_with. Imagine this test:

def test_only_the_correct_objects_call_an_api
  obj1 = SomeClass.new(ready_to_be_processed: true)
  obj2 = SomeClass.new(ready_to_be_processed: true)
  obj3 = SomeClass.new(ready_to_be_processed: false)

  SomeClass.instance_like(responds_with(:id, obj1.id)).expects(:make_api_call)
  SomeClass.instance_like(responds_with(:id, obj2.id)).expects(:make_api_call)
  SomeClass.instance_like(responds_with(:id, obj3.id)).expects(:make_api_call).never

  MakeApiCallsJob.perform_now
end

In my opinion, this is less-bad than using any_instance.

brandoncc commented 4 days ago

I should add, there isn't a MyClass.find(id) call in the job. The objects are gathered using an ActiveRecord query. I'm using job-iteration which requires the enumerator to receive an ActiveRecord::Relation. It is a lot of work to mock out one of those, and I don't love doing it. Because of that, I was trying to use something like the approach above.