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

A "targeted" version of #any_instance #653

Closed
brandoncc opened this issue Jun 26, 2024 · 5 comments
Closed

A "targeted" version of #any_instance #653

brandoncc opened this issue Jun 26, 2024 · 5 comments
Assignees

Comments

@brandoncc
Copy link

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
Copy link
Member

@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
Copy link
Author

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
Copy link
Author

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.

@floehopper
Copy link
Member

@brandoncc

Sorry for the slow response - I've been on leave. Thanks for providing more context. I understand a bit more about where you're coming from.

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

I've had some more time to think about this. I'm still pretty sceptical about it's usefulness - it's worth noting that you're the first person in over 10 years to have asked for anything like this feature.

I don't have as much time as I would like to work on Mocha and since I think this would be a non-trivial amount of work to implement, I can't see me finding the time to add it myself. The only thing that might change my mind on that is if you need this on a commercial project and your company is willing to pay for the development of the feature...

I'm not completely ruling out accepting the feature into the library, but I think it would depend a lot on how much complexity it added and how I felt about the likelihood of an additional maintenance overhead. I think I could only make that judgement call if I saw a working implementation and it feels unfair to ask you to do that without a guarantee that I would end up accepting it...

The other thing worth bearing in mind is that the code is already a lot more complicated than I would like in places - mainly because it's been around for so long and because I haven't had as much time as I would like to tidy it up. Thus I suspect it might be more dificult than you would expect to add such a feature!

I'm sorry not to be more positive, but I wanted to be honest and realistic with you.

@brandoncc
Copy link
Author

I appreciate the honesty, no need to apologize. I will close this for now, and consider writing a proof of concept if it comes back up for me.

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

No branches or pull requests

2 participants