Add Expectations Overloads for Methods With out Parameters

[MitchRazga]

First off, just want to say that this library is amazing! I've been migrating a few of my libraries from NSubstitute and I actually enjoy the process because it feels like a much cleaner syntax.

One bit of feedback I have is that the experience with mocking methods with out parameters does have a few friction points.
For example, one thing that was not immediately obvious is that a Callback() will always take precedence over ReturnValue() regardless of adornment order.

Take this method signature for example:

bool TryGet(string key, out int value);

To mock this my first thought would be to write something like:

const string ExpectedKey = "expected";
const int ExpectedValue = 10;
const bool ExpectedReturn = true;

ILookupCreateExpectations expectations = new();
_ = expectations.Methods.TryGet(key: ExpectedKey, value: ExpectedValue)
	.Callback(static (_, out valueOut) =>
	{
		valueOut = ExpectedValue;
		return default;
	})
	.ReturnValue(ExpectedReturn);

Digging into the generated code it makes sense why. Not a problem once you know.

var @result = @handler.Callback is not null ?
	@handler.Callback(@key!, out @value!) : @handler.ReturnValue;

Another noteworthy discovery is that the input validation for an out parameter always gets overridden with Arg.Any<T>(). The generated code looks something like:

internal global::Testing.ILookupCreateExpectations.Adornments.AdornmentsForHandler0 TryGet(global::Rocks.Argument<string> @key, global::Rocks.Argument<int> @value)
{
	global::Rocks.Exceptions.ExpectationException.ThrowIf(this.Expectations.WasInstanceInvoked);
	global::System.ArgumentNullException.ThrowIfNull(@key);
	global::System.ArgumentNullException.ThrowIfNull(@value);
	
	var @handler = new global::Testing.ILookupCreateExpectations.Handler0
	{
		@key = @key,
		@value = global::Rocks.Arg.Any<int>(), // See here
	};

Lastly, I feel like there is just a bit too much ceremony in setting up a mock for a method with an out parameter so where applicable in my code I write some helper methods for a cleaner look like:

internal static ILookupCreateExpectations.Adornments.AdornmentsForHandler0 TryGet(this ILookupCreateExpectations expectations, Argument<string> @key, int @value, bool @returnValue) => expectations.Methods
	.TryGet(@key, Arg.Any<int>())
	.Callback((_, out valueOut) =>
	{
		valueOut = @value;
		return @returnValue;
	});
	
	// Caller can then be like
	expectations.TryGet(key: ExpectedKey, value: ExpectedValue, returnValue: ExpectedReturn);

EDIT: I was thinking more about this and to support ref structs, a Func<T> approach is probably better:

+ internal static ILookupCreateExpectations.Adornments.AdornmentsForHandler0 TryGet(this ILookupCreateExpectations expectations, Argument<string> @key, Func<int> @value, Func<bool> @returnValue) => expectations.Methods
	.TryGet(@key, Arg.Any<int>())
	.Callback((_, out valueOut) =>
	{
+ 		valueOut = @value();
+		return @returnValue();
	});

Would be good if something like this could be easier.
I originally thought that maybe generating an overload with the CallbackForHandler delegate might help but not sure how that should look if there are multiple out parameters.

Also a nit pick for RefStructArgument<T> but it would be nice if there were some way to avoid having to use new():

bool TryGet(ReadOnlySpan<char> key, out int value);

expectations.Methods.TryGet(key: new(x => x is "expected"), value: Arg.Any<int>());

Something like a Predicate<T> generated overload or a helper similar to Arg.Validate() but for RefStructArgument<T> might be more consistent.

expectations.Methods.TryGet(key: x => x is "expected", value: Arg.Any<int>());
expectations.Methods.TryGet(key: RefArg.Validate(x => x is "expected"), value: Arg.Any<int>())

I will be using this as my sole mocking library going forward! 🎉
I do have some previous experience developing source generators so I'm happy to contribute. I'll join the Discord.

[JasonBock]

@MitchRazga Thanks for the kind comments about Rocks. While it's not my intention to have everyone move from another mocking library (like NSubstitute), it's nice to hear someone think that the Rocks experience is better. I don't get a lot of usage for Rocks so I'll take any positive comment I get :)

About a callback taking precedence over a return value setting, you're right, that is the way Rocks works, but I don't think I have that explictly documented. What I'm wondering if the right approach should be to validate this - that is, a user should not set both Callback and ReturnValue. I have a story out to add validation to the expectations that would be run when Instance() is called to ensure things are set up correctly - I could add this rule as well. That would get users to have more awareness of what's actually going on.

Having an out parameter set up to be an Arg.Any<int>() instead of the provided parameter value in expectation method ... it's because it doesn't matter what you pass in to an out parameter. The value should be supplied via a callback, so that might be why I did what I did, but if that's so, I should document it.

Your proposal (I haven't looked at your fork code yet) is intriguing. Couple of questions/concerns:

• Doing additions where overloads are added always makes me a bit concerned about overload resolution. We can't remove the current approach to not break current clients. Adding this approach, we just need to make sure that it won't cause ambiguous or confusing overload selection. I'm guessing since you said that you're already doing this with your own helper methods, my concern isn't a valid one.
• I'm assuming you'd only generate the "return value" parameter if the method in question actually returns a value.
• One unfortunate aspect about making the expectations instance have a fluent API approach is Callback is mutable. However, with your extension method, it's kind of a "one and done" shop. I don't know offhand what the best way is to handle this, but it would be nice if, with this overload approach, that these values are set and cannot be changed once the adornment instance is returned. This isn't a big deal, because, if the user is using this overload, they should know that it's setting the callback for them (this would need to be documented).
• Your point about supporting ref struct is valid, but I think we'd want to generate extension methods where the caller provides the value, and only generate Func<ref_struct_type> if the out parameter and/or the return value is a ref struct.
• If you didn't notice yet, make sure the Rocks.CodeGenerationTest app runs with TestWithTypes() being the method that's called in Program.cs. You may be doing this already, but wanted to make sure you're aware of this.

So, takeaways:

• I'll update Generate Validate() Method That Will Be Called in Mock Constructor #360 with this:
  • Add XML comments to make it clearer that setting Callback will take precedence over ReturnValue, or, potentially better, add this validation when I do (this story)[https://github.com/Generate Validate() Method That Will Be Called in Mock Constructor #360].
  • Whatever I do in the previous bullet point, add it to the overview doc.
• Please update this issue so it's retitled something like "Add Expectations Overloads for Methods With out Parameters" and base your fork/PR work on that.

Thanks again for the contribution, I look forward to reviewing what you do :).

[MitchRazga]

Thanks @JasonBock

The Validate() method sounds good but I would be concerned with how opinionated it could be. Ideally it would only validate usage of Rocks and not improper code usage like the CA2012 example in #360.

It makes sense that any input into the out parameter gets overridden with Arg.Any<T>(). I mainly only mentioned this because the ThrowIfNull still gets generated, meaning you can't input default/null.

Absolutely agree that the current approach shouldn’t be removed.
I haven’t run into any issues yet but not sure if extension methods get resolved differently. Worst case is that these potential new overloads get attributed with OverloadResolutionPriority(-1) so that the current approach doesn’t break.

I think I have convinced myself away from an returnValue: overload parameter since if a method has optional or params parameters it just gets messy.

Nothing interesting in the fork yet, I have just been getting myself familiar with these generators by experimenting with some basic mods like making nested classes partial, adding an IsAnyHandlerSet property and flattening the MethodExpectations into the main xCreateExpectations class to see if anything breaks.

I'll do some development on the overloads when I get a chance over the weekend 😄

[JasonBock]

It makes sense that any input into the out parameter gets overridden with Arg.Any<T>(). I mainly only mentioned this because the ThrowIfNull still gets generated, meaning you can't input default/null.

Yeah, that is a bit awkward. I probably implemented the out scenario and didn't really think 100% the way through it.

I did think that [OverloadResolutionPriority] may save things here if an ambiguity arises. But I'm guessing not.

Good point about optional, params, etc. This bit me with indexers and the setter where I had to move the supplied value first in the parameter list. It's a bit awkward, but there was no other way to do that if optional values existed. We might do the same thing here, but I also am not a fan of it either. But if we don't provide a "return value" parameter, then we run into the situation where the Callback will be set, then the user has to set ReturnValue, and then we're in a situation where Rocks currently will only invoke Callback if it's set. This could get messy too.

[JasonBock]

I think if this is done, we should merge this issue in with this, because this will inevitably get in the way of this work (probably), so, probably want to do this to iron out any issues that may occur if they're done separately.