Ensure lack of optional parameters

[nulltoken]

Fix #952

• Create a MetaFixture ensuring no optional parameter is used
• See the test fail
• 🔥 the optionals
• See the test pass

[shiftkey]

😍 😍 😍 😍 😍

@nulltoken did you want an extra pair of hands to help with cleaning this up? otherwise I'm happy to help with reviewing...

[shiftkey]

typo: parameter

[nulltoken]

@shiftkey Any help is welcome killing those optional parameters! 🙏

[shiftkey]

@nulltoken I'll start from the bottom of the list then. Shall I PR into this PR, or just add commits directly?

[nulltoken]

@shiftkey Please push directly on top of this branch. ❤️ for the help

[shiftkey]

There's a couple of methods which have optional parameters but are marked as obsolete. I figure we don't care about migrating those. Let me know if that's not the case.

[nulltoken]

Good call! They'll be dropped before stabilization (post v0.22)

[shiftkey]

Not sure if we wanted to overload everything here - I just went with the overloads for global and global/xdg. Would love some guidance.

[jamill]

How you did it makes sense to me.

[shiftkey]

👍

[shiftkey]

Pausing here because I'm concerned about wrestling with the mocks for Configuration and Doing It Wrong™ - the new tests however are passing, and I Have Questions™ about the ctor overloads for Configuration.

[jamill]

😍

[shiftkey]

@nulltoken thoughts on what I'm doing wrong with that leaked ConfigurationSafeHandle - I can't seem to trigger this when running the CanFakeEnumerationOfConfiguration test locally?

[jamill]

@shiftkey see this line - In order to trigger this, you need to have LEAKS_IDENTIFYING defined. uncomment that line, or add that symbol. Here is the trace with LEAKS_TRACKING specified:

at LibGit2Sharp.Core.NativeMethods.git_config_new(ConfigurationSafeHandle& cfg)
at LibGit2Sharp.Core.Proxy.git_config_new() in C:\Users\jamill\gitWorkspaces\libgit2sharp\LibGit2Sharp\Core\Proxy.cs:line 482
at LibGit2Sharp.Configuration.Init() in C:\Users\jamill\gitWorkspaces\libgit2sharp\LibGit2Sharp\Configuration.cs:line 46
at LibGit2Sharp.Configuration..ctor(Repository repository, String globalConfigurationFileLocation, String xdgConfigurationFileLocation, String systemConfigurationFileLocation) in C:\Users\jamill\gitWorkspaces\libgit2sharp\LibGit2Sharp\Configuration.cs:line 41
at LibGit2Sharp.Configuration..ctor(String globalConfigurationFileLocation, String xdgConfigurationFileLocation, String systemConfigurationFileLocation) in C:\Users\jamill\gitWorkspaces\libgit2sharp\LibGit2Sharp\Configuration.cs:line 100
at Castle.Proxies.ConfigurationProxy..ctor(IInterceptor[] , String , String , String )
at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
at System.Reflection.RuntimeConstructorInfo.Invoke(BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
at System.RuntimeType.CreateInstanceImpl(BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture, Object[] activationAttributes, StackCrawlMark& stackMark)
at System.Activator.CreateInstance(Type type, BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture, Object[] activationAttributes)
at System.Activator.CreateInstance(Type type, Object[] args)
at Castle.DynamicProxy.ProxyGenerator.CreateClassProxyInstance(Type proxyType, List`1 proxyArguments, Type classToProxy, Object[] constructorArguments)
at Castle.DynamicProxy.ProxyGenerator.CreateClassProxy(Type classToProxy, Type[] additionalInterfacesToProxy, ProxyGenerationOptions options, Object[] constructorArguments, IInterceptor[] interceptors)
at Moq.Proxy.CastleProxyFactory.CreateProxy(Type mockType, ICallInterceptor interceptor, Type[] interfaces, Object[] arguments)
at Moq.Mock`1.<InitializeInstance>b__2()
at Moq.PexProtector.Invoke(Action action)
at Moq.Mock`1.InitializeInstance()
at Moq.Mock`1.OnGetObject()
at Moq.Mock`1.get_Object()
at LibGit2Sharp.Tests.MockingFixture.CanFakeEnumerationOfConfiguration() in C:\Users\jamill\gitWorkspaces\libgit2sharp\LibGit2Sharp.Tests\MockingFixture.cs:line 97
at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments)
at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
at Xunit.Sdk.Reflector.ReflectionMethodInfo.Invoke(Object testClass, Object[] parameters)
at Xunit.Sdk.FactCommand.Execute(Object testClass)
at Xunit.Sdk.FixtureCommand.Execute(Object testClass)
at Xunit.Sdk.BeforeAfterCommand.Execute(Object testClass)
at Xunit.Sdk.LifetimeCommand.Execute(Object testClass)
at Xunit.Sdk.ExceptionAndOutputCaptureCommand.Execute(Object testClass)
at Xunit.Sdk.TimedCommand.Execute(Object testClass)
at Xunit.Sdk.TestClassCommandRunner.Execute(ITestClassCommand testClassCommand, List`1 methods, Predicate`1 startCallback, Predicate`1 resultCallback)
at Xunit.Sdk.Executor.RunTests.<>c__DisplayClass12.<.ctor>b__f()
at Xunit.Sdk.Executor.ThreadRunner(Object threadStart)
at System.Threading.ThreadHelper.ThreadStart_Context(Object state)
at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
at System.Threading.ThreadHelper.ThreadStart(Object obj)

[jamill]

@shiftkey - I am not completely familiar with this yet, but it looks like the Mock<Config> object is calling the constructor for the class, which in turn allocates a new ConfigurationSafeHandle. My (possibly wrong) expectation is that we do not want the mock to actually initialize itself to this extent? Having the mock call into libgit2 and set up some native structures seems to break the encapsulation of the mock object - maybe we want an IConfig interface?

[shiftkey]

Okay, that should be the fix. I'll try and give it thorough review to ensure the optional combinations are maintained wherever appropriate, but any extra help would be 💖

[shiftkey]

[nulltoken]

What does this change do? (asking for a friend)

[shiftkey]

I think I found myself dropping the default _ when R# was bugging me during the refactoring. I'll check.

[nulltoken]

Pushed some fixups

[nulltoken]

Anyone opposing to see this pulled in?

[Therzok]

[nulltoken]

@shiftkey AWE. SOME! 💯 🎱