Support for Mock.Of<T> with not default constructor

[JorTurFer]

At the moment, new Mock and Mock.Of are similar but if you need use non generic constructor you must use new Mock. If you are using the Mock.Of syntax in the project, change all for this will be a problem.
Add support for this is a very little change and give more functionality to the Mock.Of syntax.

[JorTurFer]

Any new about this?

[stakx]

@JorTurFer, I'd like to apologize to you for the radio silence. I've seen your issue and the PR, but haven't got around to dealing with it (and other issues)... see #966 for the reason why. Sorry again for the delay.

[JorTurFer]

@stakx dont worry !! I wrote a comment to do ping, but you should enjoy the vacation. It's mandatory !! The repository will be here when you come back and the world will continue xD.

[JorTurFer]

Hi @stakx , any new about this?

[peterdew]

I looked into the pull and did a review just to support this addition @JorTurFer since it is good work! Hopefully you agree with my reviewcomments @stakx

[stakx]

@JorTurFer once again sorry for the long silence. I've prioritized some other work and have been busy over at the DynamicProxy repo so we'll eventually get a full netstandard2.x target all the way down.

@peterdew, thanks for jumping in and doing a code review. I agree that it looks like a solid & well done PR.

My (perhaps somewhat unreasonable?) gripe is with params object[]:

Moq's public API is chock-full with method overloads, and we keep adding more all the time. This is mostly fine but care needs to be taken in those cases to not break existing code by making existing code suddenly wind up with a different overload when compiling after a Moq update.

A single params object[] parameter seems especially dangerous because it can in theory be used for any combination of parameters... and the only thing saving us from that trap is C#'s overload resolution rules that favor different, more specific overloads... I think.

It is this last bit I am worried about: can we prove somehow that existing code won't suddenly end up in the new method when it called a different one before?

Second, I think the new feature should be rephrased slightly because "non-generic constructor" suggests something that doesn't exist: ctors are never generic; it's really the generic class Mock<T> you appear to be referring to. At the same time this feature isn't "non-generic" at all, we're just using a generic factory method instead of the ctor of a generic type.

Finally, and perhaps that's the most fundamental question I'm asking myself here: does this addition actually add value? Why do we need two separate APIs for creating mocks? Remember that Mock.Of<T> used to have the specific purpose of supporting LINQ-like specifications and it was a completely separate approach to using Setup; these distinctions have largely disappeared but even so, I have to wonder if Mock.Of<T> needs to have all the same overloads that Mock<T>..ctor has? Why not simply use the existing ctor if you're not going to use a LINQ specification anyway?

I'm undecided on many of these points and regarding the overload resolution would need to take a closer look; I just thought I'd let you know my thoughts.

[JorTurFer]

Thanks for share your thoughts @stakx.
I think the same as you about the use of two different APIs to create mocks and maybe best solution is to deprecate Mock.Of in favour of new Mock. I suggest this feature becouse if Mock.Of is not deprecated a lot of people could use it (for example me).
If we asume the scenario with 2 different APIs active, the problem with params object[] is not a real problem becouse you are right when you say that the compiler use the most specific overload and non parameter overload is more specific than params.
Personaly, I alwais use Mock.Of becouse the code is more clear and the use is more direct but it is just an opinion.

[stakx]

@JorTurFer, to be clear, IIRC Moq v5 is gravitating more towards something like Mock.Of so if we deprecated sth it would more likely be new Mock — but IMHO we cannot deprecate either in Moq v4, since they serve different purposes.

I'm willing to merge your PR despite my misgivings about the API redundancy, BUT I still think we need to be somewhat more systematic about the overload issue; I think we shouldn't just hope for the best, but instead try to identify some scenarios/instances where the new overload could be problematic (if any exist, that is).

[JorTurFer]

Sorry @stakx , when I said deprecate I wanted to say mark as obsolete (I don't know what I was thinking about...).

My experience said that it is not a problem but you are right about the overloads' posible danger.
I was reviewing the code and I can see that new Mock also has params and in case of fail on overload candidate selection, we will call new Mock with a lenght zero params array and this is also correct.
I am going to add some tests those cover this situation in order to be sure that is not a problem and I will update my PR if the tests are correct.

[JorTurFer]

Hi @stakx and @peterdew ,
I pushed some changes into the PR to solve the problems of the code reviews and I added 2 tests to validate the case of the hypotetic fail in the overload candidate selection.
Tell me if you have something :)

[stakx]

Closing this as "unresolved", the accompanying PR has since been closed, and this enhancement does not have priority at this time.