Open floehopper opened 9 years ago
I'm not sure I understand custom actions properly. With my limited understanding, I suspect #379 may provide the described capability.
If not, I'd love to help implement this feature if you could explain to me the desired behavior. A ruby/mocha hypothetical example would be great.
I've changed the title of this issue, because I want to use it to roll up a number of closely related issues and have the discussion in one place.
I'm not sure I understand custom actions properly. With my limited understanding, I suspect #379 may provide the described capability.
If not, I'd love to help implement this feature if you could explain to me the desired behavior. A ruby/mocha hypothetical example would be great.
@nitishr Thanks for asking about this.
You're right that #213, #230, #231, #317 and your latest #379 are all closely related to this issue. I'm going to close all those and continue the discussion here.
My hesitation about implementing this seemingly innocuous extra functionality is that it seems to violate the idea of a stub always returning "canned" data and thus risk the test becoming very complicated and hard to follow.
Passing a block or proc to be invoked by the stub means allowing any amount of arbitrary code to be executed by the stub including the possibility of mutating state outside the stub or even global state. This feels inherently dangerous and I can imagine people getting in a real mess with it.
I think that the execution flow of a test should go from the test (T) to the object under test (O) and then into one or more stubbed methods (S) and then back out to the object under test and ultimately the back to the test again:
T -> O
O -> S
O <- S
T <- O
However, if you allow an arbitrary block to be invoked by the stub, you're likely to end up with something more like:
T -> O
O -> S
T <------ S
T ------> S
O <- S
T <- O
And I think this would quickly become hard to follow.
My thinking on this is somewhat influenced by the mess many people get into by using Mocha::ClassMethods#any_instance
. I think this is rarely needed and is often the sign of a problem with the design, although in the case of testing legacy code or code that you don't have control over, I can see it has benefits. I sometimes think it might make sense to have a separate "legacy code" mode for Mocha which enables some features which are disabled by default. This is somewhat related to the stubbing_method_on_non_mock_object
configuration option.
The reason I thought basing functionality like this on jMock's custom actions was it might be a way to constrain the functionality in a way that made it clear how it was meant to be used and make it less likely people would get in a mess.
Looking through the block implementation examples in the rspec-mocks documentation it feels as if most of those scenarios can be handled in another way:
Mocha::Expectation#with
.Mocha::Expectation#with
. I can see how this functionality could reduce duplication if you're stubbing the calculation method many times, but I think the same reduction of duplication could be achieved by extracting a method to do setup the stubbing.Mocha::Expectation#yields
and/or Mocha::Expectation#multiple_yields
.Mocha::Expectation#raises
and/or Mocha::Expectation#returns
.Thus I don't currently see a strong argument for introducing the functionality. If anything, I think the best solution might be to improve the documentation to provide suggested solutions for the examples in the rspec-mocks docs and examples from some of the PRs & issues related to this one.
I welcome any thoughts people have on the subject.
@floehopper My use case for this is usually to replace a function that does IO with a function that returns mock data based on the input arguments. The mock function is often as simple as serializing the input arguments and using the result to return a value from a hash. Is there a simple way to achieve this without the ability to provide the mock implementation?
@Hubro Interesting. Any chance you could post some simple example code here, so I know exactly what you mean?
@floehopper The simplest usecase for this I can think of is dynamically controlling the returned value as a function of the argument passed in when that transformation cannot be modeled as a limited set of if-then arguments. Like what if I want to stub a method to just return what's passed in? E.g.
Foo.stubs(:bar) do |arg_1|
arg_1
end
etc.
I have another example where I need this. I need to stub a method that could take some time, and I want to call the rails travels
time helper to simulate the method taking a while
I have another example where I need this. I need to stub a method that could take some time, and I want to call the rails
travels
time helper to simulate the method taking a while
@nhorton Hmm. That's an interesting use case. Do you have a more concrete example of where it's important that the stubbed method looks like it's taking some time to execute - ideally some actual test and implementation code. If I can't think of a way to redesign the implementation to make it easier to test, then I'll definitely consider adding a feature to Mocha to do this, although I'd probably lean towards adding something very specific to this use case (e.g. foo.stubs(:bar).after(5.seconds).returns(:bar)
rather than allowing arbitrary block evaluation.
@floehopper The simplest usecase for this I can think of is dynamically controlling the returned value as a function of the argument passed in when that transformation cannot be modeled as a limited set of if-then arguments. Like what if I want to stub a method to just return what's passed in? E.g.
Foo.stubs(:bar) do |arg_1| arg_1 end
etc.
@aditya87 I'm so sorry that it's taken me sooo long to reply to your comment. I'm really looking for a more concrete example than that - ideally both test and implementation code. That way I can see if the problem can be addressed by redesigning the implementation so that it can be tested with the existing Mocha features. If I can't then that's more motivation to add a feature to Mocha and would give me a clearer idea of the problem I'm trying to solve with that feature. I hope that makes sense.
Here is an example. I think this might be worst-case scenario too. The functionality in question here is a soft-timeout on jobs where they end and re-enqueue themselves (with checkpoints) when an inner loop has taken longer than the allowed time. The 37Signals blog has an article on this approach in their blog. This code is stupid-dumb because I need to keep everything in the same context to get the time travel functionality in Rails test to work properly.
The test code is awkward as I wanted to use a Mocha mock object but had to pivot to injecting a module into Integers when Mocha did not work.
module NumericWarmeable
def warm_record_cache!(async:)
raise "Should not be async" if async
stubbed_travel self.seconds # Travel is the rails time travel testing helper
end
# This has to exist to be stubbed, and we need to stub it with a proc for scope purposes
def stubbed_travel(seconds) = 1
end
test "the soft time limits work - slow tasks" do
instant = warmeables(:example_1)
Warmeable.any_instance.expects(:warm_record_cache!).times(2)
# This is dumb-complex to test as we have to stub awkwardly to have the time travel work
too_long = 10.minutes.seconds
too_long.extend(NumericWarmeable)
mock_warm = proc { |seconds| travel seconds }
too_long.stub(:stubbed_travel, mock_warm) do
assert_enqueued_jobs(1, only: CacheWarmingJob) do
CacheWarmingJob.perform_now(@account, [instant, too_long, instant])
end
perform_enqueued_jobs(only: CacheWarmingJob)
assert_enqueued_jobs(0, only: CacheWarmingJob)
end
end
@nhorton Thanks. I hope you don't mind but I've fixed the indentation in your code example. Also I think I'm missing a few things here to be able to understand & run this code, e.g. the definitions of Warmeable
, CacheWarmingJob
, #warmeables
.
Scan https://dev.37signals.com/making-export-jobs-more-reliable/ for a minute. That is the core of what I am doing.
To directly answer, Warmeable in my example is a basically a delegated type that needs a bunch of computations done on it. The job in question does the computations on a set of objects. The issue is that some computations are fast and some are slow (they call out to remote systems ) and I need to simulate different response times. But because I can't properly do a mock, and because Active job serialization does not support arbitrary types, I ended up extending int in a stupid-complex way trying to get something that could stub with a proc that has the rails time travel methods in scope and would serialize.
On Mon, Sep 4, 2023, 2:47 PM James Mead @.***> wrote:
@nhorton https://github.com/nhorton Thanks. I hope you don't mind but I've fixed the indentation in your code example. Also I think I'm missing a few things here to be able to understand & run this code, e.g. the definitions of Warmeable, CacheWarmingJob, #warmeables.
— Reply to this email directly, view it on GitHub https://github.com/freerange/mocha/issues/224#issuecomment-1705689609, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABR24TJ77I7OHXIJ2ELQM3XYY46XANCNFSM4A6SFUFA . You are receiving this because you were mentioned.Message ID: @.***>
Hmm. It would really help me if you could at least sketch out the classes/modules under test, even if the method bodies are just comments explaining roughly what they do. What might seem obvious to you, isn't to me!
Also I'm confused by this method call:
too_long.stub(:stubbed_travel, mock_warm) do
# ...
end
That stub
doesn't seem to be a legitimate Mocha method and even if it's a typo of stubs
, that doesn't accept a block. Or am I missing something...?
Explaining my code will never be ideal as there is too much domain stuff. Let me do a simpler synthetic version with the code I would like to write.
ImageService.capture_frame(video_name)
An example test that I can write now is on the daily reset. Roughly
test "Test that the reset happens right at midnight regardless of when the last call was" do
travel_to Time.current.midnight - 1.hour
test_account.over_daily_limit = true
travel_to Time.current.midnight + 1.second
assert_false test_account.over_daily_limit
end
But what I can't do is something that stubs that service for testing my tracking.
test "When I cross 5 minutes with ImageService, the account is marked over its limit" do
ImageService.stubs(:capture_frame).latency(30.seconds).returns("https://fake.url/image.jpb")
test_account.usage_today = 4.minutes + 45.seconds
test_account.encode_another_video # This is the method that internally calls ImageService.capture_frame
assert_true test_account.over_daily_limit
end
Thanks for having follow-up here.
See http://www.jmock.org/custom-actions.html and https://relishapp.com/rspec/rspec-mocks/docs/configuring-responses/block-implementation.