[RFC] Remove the code we have just for mocking#1272
Conversation
We make the constructors internal but then make sure we cave constructors with zero arity so mocking them via e.g. Moq is easier. With this we're making out code harder to mock, but then add ways to add another layer of mocking easier. We enforce this constructors for mocking in the tests, without regard for whether they make sense or not. Often it would make a lot more sense to expose a public constructor for types so an application can return fake results.
Yeah, in many cases that's true. But looking at this change briefly, it looks like it just removes protected constructors without adding public constructors.
Yeah. Typically I'll have a method in my app that returns (or accepts) a LibGit2 object and I want to abstract the call to LibGit2 so I don't actually have to do a git operation. For example, public int GetRepositoryIsAwesomeScore(LibGit2Sharp.Repository repository)
{
// Does stuff with repository. Returns a score.
return 100; // all my repos are awesome.
}So now, I want to write a test of my method. If I can't instantiate or mock This is one reason I like the In C#, as you know, classes are not sealed by default, but methods are. So a non-sealed class with only internal constructors is just the work. It offers the promise that I can override it, but I can't. I would love it if every class implemented an interface (and the API returned the interfaces). Then I wouldn't care if every class had internal constructors or if the classes were sealed. Does that make sense? |
|
I understand that you don' t want to create a wrapper class for every LibGit2Sharp type. But isn't that roughly the same problem as creating an interface for every LibGit2Sharp type? |
No, because there are great frameworks for dynamically implementing an interface. For example, suppose my method is: public class RepositoryScorer {
public int GetRepositoryIsAwesomeScore(LibGit2Sharp.IRepository repository) {
return repository.Name == "haacked.com" ? 100 : 0;
}
}My test is var fixture = new RepositoryScorerer();
var repo = Substitute.For<IRepository>();
repo.Name.Returns("haacked.com");
var result = fixture.GetRepositoryIsAwesomeScore(repo);
Assert.Equal(100, result);Note that this is a super dumb simple example, but hopefully it conveys the point. I'm not interested in testing the internals of LibGit2Sharp, I assume you do that. 😄 I just want to test my interactions with LibGit2Sharp. |
|
@haacked I guess I'm not up to speed on the state of the art when it comes to .NET mocking and dependency injection. I've found tools on other platforms (eg Mockito) to be reasonably capable of mocking concrete classes in the past. Certainly we should not make it too difficult for consumers to mock us. But if this is a limitation of a particular DI library then I'm less excited about 1:1 interfaces:classes. |
It's not a limitation of a particular DI library, it's a limitation of the CLR and the design of LibGit2Sharp. I'll try and explain. Mockito is for Java, right? In Java, methods are virtual by default. So mocking a typical class works because a typical class is not sealed. But if you seal a jar for example, Mockito will fail. In .NET, it is possible to mock concrete classes if you can instantiate them and if the methods you're mocking are virtual. But if you make the constructor internal, I can't instantiate them without resorting to hacks such as private reflection. That's one of the reasons we advocated adding protected parameterless constructors. It lowers the difficulty in creating automatic mocks. In theory, I could use the profiling API which basically hijacks the runtime and lets me override anything, but that's a heavyweight approach and is just working around a design that provides very little seams for testing. |
|
The repository is a bit of a catch-all, so there's probably no way around mocking or having an interface, but if we make the results and status objects have a constructor so you can create arbitrary ones, you wouldn't need to mock whatever's creating them, since you can have whatever you're doing return that result without calling out to the library. I'm hoping this is enough for those classes which are are basically structs we have made classes. The examples of mocking in C# I've seen all seem to be a mess, so if like to have a look at our usage to see if it matches what I'm thinking. |
This term is overloaded so it's hard to tell if you're talking about this repository or the
Yeah, which is why I'm very happy we have
Yeah, at the end of the day, if I can create the object without having to use private reflection, then I'm happy. This isn't true for all cases. For example, some objects are hard to construct and require building whole object graphs which can get annoying when I really just want to isolate a slice of the object.
Yeah, that makes sense. Again, the main problem right now is that I can't instantiate them with the values I want. With a typical struct, I can do that. That's why I'm not going around asking people to add interfaces for their structs. 😄 |
|
I did mean the Repository class :) When you need a whole graph of objects, then that does get trickier, but my
|
Cool. I think that'll help, but I'll have to see examples. As you can imagine, GitHub Desktop's use case is a bit more involved than most where we often need to explore the structure of the |
Instead of hiding everything but making it virtual for mocking, let the users create their own objects with the data they want.
|
I've made anything |
|
My next step would be to fold the |
We should not have extensions on our own types. Either we accept these convenience overloads as part of our interfaces or we shouldn't have them. While here unseal Repository and split up these methods into a few different files grouping them by area.
3 participants
We make the constructors internal but then make sure we cave
constructors with zero arity so mocking them via e.g. Moq is
easier. With this we're making out code harder to mock, but then add
ways to add another layer of mocking easier.
We enforce this constructors for mocking in the tests, without regard
for whether they make sense or not. Often it would make a lot more sense
to expose a public constructor for types so an application can return
fake results.
I'm thinking specifically that what you would want to do 90% of the time is mock your own app (if at all) and then return a fake
...Entryor...Resulttype, and we can provide constructors for those so we treat them more like a struct than a class with actual scoping./cc @haacked and @shana for thoughts on mocking and whether something might actually break and what we can do to fix it.