Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Version 2.4.x System.ArgumentNullException parsing options in unit-test #389

Closed
mbedded opened this issue Jan 15, 2019 · 19 comments
Closed

Comments

@mbedded
Copy link

mbedded commented Jan 15, 2019

Hello,

i tried all 2.4.x versions (2.4.0, 2.4.1, 2.4.2, 2.4.3) and all have the same exception. In my unit-test i call my program only with "--help" after that, i receive the following stacktrace:

// Stacktrace
System.ArgumentNullException
Value cannot be null.
Parameter name: element
   at System.Attribute.GetCustomAttributes(Assembly element, Type attributeType, Boolean inherit)
   at System.Reflection.CustomAttributeExtensions.GetCustomAttributes[T](Assembly element)
   at CommandLine.Infrastructure.ReflectionHelper.GetAttribute[TAttribute]()
   at CommandLine.Text.HelpText.AutoBuild[T](ParserResult`1 parserResult, Func`2 onError, Func`2 onExample, Boolean verbsIndex, Int32 maxDisplayWidth)
   at CommandLine.Text.HelpText.AutoBuild[T](ParserResult`1 parserResult, Int32 maxDisplayWidth)
   at CommandLine.Parser.<>c__DisplayClass17_0`1.<DisplayHelp>b__1(IEnumerable`1 _, TextWriter writer)
   at CSharpx.MaybeExtensions.Do[T1,T2](Maybe`1 maybe, Action`2 action)
   at CommandLine.ParserResultExtensions.WithNotParsed[T](ParserResult`1 result, Action`1 action)
   at CommandLine.Parser.DisplayHelp[T](ParserResult`1 parserResult, TextWriter helpWriter, Int32 maxDisplayWidth)
   at Program.<Main>d__5.MoveNext() in C:\..\..\Program.cs:line 33

My code snippets are the following ones:

// Test method (xUnit) which fails
[Fact]
public async Task CallMain_GiveHelpArgument_ExpectSuccess() {
    var result = await Program.Main(new[] { "--help" });

    Assert.Equal(ERROR_SUCCESS, result);
}
// main program
internal class Program {

  private const int ERROR_SUCCESS = 0;

  internal static async Task<int> Main(string[] args) {
      bool hasError = false;
      bool helpOrVersionRequested = false;

      ParserResult<Options> parsedOptions = Parser.Default.ParseArguments<Options>(args).WithNotParsed(errors => {
          helpOrVersionRequested = errors.Any(x => x.Tag == ErrorType.HelpRequestedError || x.Tag == ErrorType.VersionRequestedError);
          hasError = true;
      });

      if (helpOrVersionRequested) {
          return ERROR_SUCCESS;
      }

    // Execute as a normal call
    // ...
  }
  
}
// Options
internal class Options {

  [Option('c', "connectionString", Required = true, HelpText = Texts.ExplainConnection)]
  public string ConnectionString { get; set; }

  [Option('j', "jobId", Required = true, HelpText = Texts.ExplainJob)]
  public int JobId { get; set; }

  [Usage(ApplicationAlias = "Importer.exe")]
  public static IEnumerable<Example> Examples {
      get => new[] {
          new Example(Texts.ExplainExampleExecution, new Options() {
              ConnectionString="Server=MyServer;Database=MyDatabase",
              JobId = 5
          }),
      };
  }

}

I did a downgrade to version 2.3.0 without any issues. Calling the compiled Program.exe by a command line seems to be working as well. I created that test to ensure my application stops when the version or help is requested only without any additional information.

@mbedded mbedded changed the title Version 2.4.x System.ArgumentNullException parsing options Version 2.4.x System.ArgumentNullException parsing options in unit-test Jan 15, 2019
@ericnewton76
Copy link
Member

Thats interesting... 3 levels deep into BCL reflection code.

Are you running on framework 4.7.2 or net core or what?

@ericnewton76
Copy link
Member

So this works on my machine, running xunit.

Also I checked in your test code into branch fix/issue-389 and the build server ran the test (look for issue389_tests) https://ci.appveyor.com/project/commandlineparser/commandline/build/job/o3uhjxvn8xdha46g/tests

Not sure why you'd get this error...

@mbedded
Copy link
Author

mbedded commented Jan 18, 2019

I am using a Console-Application with .Net Framework 4.7.2. thanks for your investigation. I will re-check this.
I am unsure, too. The only thing i did was upgrading the Nuget Package. I did it isolated (only update) and cleaned the bin + obj folder to ensure the cache is not messing up anything.

I'll can give you a feedback to this within next week.

@moh-hassan
Copy link
Collaborator

moh-hassan commented Jan 20, 2019

TL;DR,

I could reproduce the same error in a test project of target framework net472 , but sucess in netcoreapp2.1 using commaline v2.4.3.

This problem is due to the behavior of testhost in Full framework net46.x / net4.7.x and netcore2.x in reading AssemblyAttributes.

This a long story and I have previously posted an issue to vstest project to correct that behavior.

Commandlineparser consider AseemplyAttributes copyright and company shouldn't be null and raise exception (I think it should be relaxed), which may be null in hosttest and cause troubles with commandlineparser.

To workaround the problem in net472 (compliant with netstandard2)
let test classes inherit the following class:

    public class BaseTest
   {
       public BaseTest()  
      {  
	//in case multi target project,use #if because no appdomain in netcore
	#if NET472
        /* Preparing test start */  
        Assembly assembly = Assembly.GetCallingAssembly();  

        AppDomainManager manager = new AppDomainManager();  
        FieldInfo entryAssemblyfield = manager.GetType().GetField("m_entryAssembly", BindingFlags.Instance | BindingFlags.NonPublic);  
        entryAssemblyfield.SetValue(manager, assembly);  

        AppDomain domain = AppDomain.CurrentDomain;  
        FieldInfo domainManagerField = domain.GetType().GetField("_domainManager", BindingFlags.Instance | BindingFlags.NonPublic);  
        domainManagerField.SetValue(domain, manager);  
        /* Preparing test end */  
	#endif
    } 
  }

   //your test class inherit BaseTest
    public class TestProgram: BaseTest
	{
		// Test method (xUnit) which fails and really fail when inheriting the BaseTet class
		[Fact]
		public async Task CallMain_GiveHelpArgument_ExpectSuccess() {
		var result = await Program.Main(new[] { "--help" });
                    int ERROR_SUCCESS = 0;
		Assert.Equal(ERROR_SUCCESS, result);
	  }
	}	

I tested your code in multi-target project in Xunit for net472 and netcoreapp2.1 and both are sucess and NO exception is raised.
Run:

   dotnet test

The output in the multi-target test project is:

Test run for F:\github_issues\CommandParser432\CommandParser432\bin\Debug\net472\CommandParser432.dll(.NETFramework,Version=v4.7.2)
Microsoft (R) Test Execution Command Line Tool Version 15.9.0
Copyright (c) Microsoft Corporation. All rights reserved.

Starting test execution, please wait...

Total tests: 1. Passed: 1. Failed: 0. Skipped: 0.
Test Run Successful.
Test execution time: 7.9740 Seconds
Test run for F:\github_issues\CommandParser432\CommandParser432\bin\Debug\netcoreapp2.1\CommandParser432.dll(.NETCoreApp,Version=v2.1)
Microsoft (R) Test Execution Command Line Tool Version 15.9.0
Copyright (c) Microsoft Corporation. All rights reserved.

Starting test execution, please wait...

Total tests: 1. Passed: 1. Failed: 0. Skipped: 0.
Test Run Successful.
Test execution time: 5.0017 Seconds

Conclusion:

The xunit test project is working fine when you inherit the TestBase class in net472.

@JVimes
Copy link

JVimes commented Jan 24, 2019

I have the exception with NUnit. I think it's because Assembly.GetEntryAssembly() returns null on this line.

Here's a simple repro project: IssueDemo.zip (might have to build, clean, rebuild since NUnit test adapter is quirky).

@moh-hassan
Copy link
Collaborator

moh-hassan commented Jan 25, 2019

I test your demo and it fail with the error message

Error Message:
System.ArgumentNullException : Value cannot be null.
Parameter name: element

As a workaround solution, I modified your project by inheriting the class `BaseTest' that i described in my comment
and it pass successfully.

Total tests: 1. Passed: 1. Failed: 0. Skipped: 0.
Test Run Successful.

The root error is that the Assemplyattribute copyright and company are null in the hosttest of vstest which is used by NUnit and Xunit, so Commandlineparser raise an exception when execute Assembly.GetEntryAssembly() .

This bug is in net4x only. In netcoreapp2.x test pass successfully.
So it's best to test project in netcoreapp2.x which use the same packages (work only in SDK style project )

IssueDemo-sucess.zip

@MeikelLP
Copy link

I can confirm this issue using Unity 2018.3.0f1 (.NET standard 2.0).

@MeikelLP
Copy link

MeikelLP commented Jan 25, 2019

Issue seems to linked to Assembly.GetEntryAssembly() returning null. According to Microsoft this can only happen if any unmanaged assembly is loaded.

Some quick tests:

Debug.Log(Assembly.GetEntryAssembly()); // null
Debug.Log(Assembly.GetExecutingAssembly()); // my assembly
Debug.Log(Assembly.GetCallingAssembly()); // my assembly

I don't know too much of reflection that's why I don't know why you're using the entry assembly instead of the executing or calling one. Can you please explain?

@MeikelLP
Copy link

Probably the issue is the method GetExecutingOrEntryAssmbly which always returns the entry assembly?

private static Assembly GetExecutingOrEntryAssembly()
{
return Assembly.GetEntryAssembly();
}

@ericnewton76
Copy link
Member

Hmmm. Interesting. Its odd to be running commnadlineparser through unity though... I dont know much about it, but seems counterproductive since (I'm guessing) unity has its own bootloader before your library starts running? Thats the only reason I can surmise, without more information, about how GetEntryAssembly would return null.

Can you provide more info about how this is being run?

@MeikelLP
Copy link

Yes you're right unity has it's own native bootloader calling my library. This is the default unity way. I don't think there is a way around this in any way without modifying the engine itself.

I need multiple unity instances started with custom command line args each. I thought it would be wise to use a library for this purpose but it seems to conflict with unity's arguments like I mentioned in my own issue.

@JVimes
Copy link

JVimes commented Jan 28, 2019

Just a reminder this exception happens without Unity.

@mbedded
Copy link
Author

mbedded commented Jan 29, 2019

@moh-hassan thank you for your workaround. That fixed my tests.
Maybe we have to handle this as "given by the framework". Unfortunately i cannot move to .NET Core yet because we have many internal projects to update as well.

This workaround works good for .NET Framework. Thank you again for providing a solution.

@ericnewton76 i created a unit test to ensure the application exits after e.g. version or help was used. Furthermore it should exit when some parameters are missing. This is needed to cover my main-method. Only if the startup parameters are correct i want to start the real actions.

Maybe this ticket can be closed? When i compile my console-project using .NET Framework 4.7.2 and call it via CMD it is executed as expected. It seems the failure is caused by my Unit-Test project.

@ericnewton76
Copy link
Member

@mbedded when you say "Furthermore it should exit when some parameters are missing"... eh, yes and no... realistically some implementations can move forward with what they have.

It's really left up to the implementation to decide if thats actually the case, and we shouldn't push that choice out onto the library consumers.

You mentioned you added a unit test. Thanks for that! I'll look into this.

@ericnewton76 ericnewton76 added merge-pending This PR looks good and most likely gets into next release and removed merge-pending This PR looks good and most likely gets into next release labels Feb 1, 2019
@franklin-ross
Copy link

franklin-ross commented Mar 18, 2019

Looks like I'm getting this same error in LINQPad too, which is fairly annoying. In this case it might not have anything to do with native assemblies, but more to do with dynamic compilation of queries? Here's a minimal repro script (needs the library from NuGet of course):

class Options
{
    [Option('o', Required = true)]
    public bool Opt { get; set; }
}

void Main(string[] args)
{
    Parser.Default.ParseArguments<Options>(args ?? new string[0])
        .WithParsed(options => options.Dump());
}

Gives an ArgumentNullException(Value cannot be null.Parameter name: element) with the following stack trace:

at System.Attribute.GetCustomAttributes(Assembly element, Type attributeType, Boolean inherit)  
at System.Reflection.CustomAttributeExtensions.GetCustomAttributes[T](Assembly element)  
at CommandLine.Infrastructure.ReflectionHelper.GetAttributeTAttribute  
at CommandLine.Text.HelpText.AutoBuild[T](ParserResult1 parserResult, Func2 onError, Func2 onExample, Boolean verbsIndex, Int32 maxDisplayWidth)   at CommandLine.Text.HelpText.AutoBuild[T](ParserResult1 parserResult, Int32 maxDisplayWidth)  
at CommandLine.Parser.<>c__DisplayClass17_01.<DisplayHelp>b__1(IEnumerable1 _, TextWriter writer)   at CSharpx.MaybeExtensions.Do[T1,T2](Maybe1 maybe, Action2 action)  
at CommandLine.Parser.<>c__DisplayClass17_01.<DisplayHelp>b__0(IEnumerable1 errors)  
at CommandLine.ParserResultExtensions.WithNotParsed[T](ParserResult1 result, Action1 action)  
at CommandLine.Parser.DisplayHelp[T](ParserResult1 parserResult, TextWriter helpWriter, Int32 maxDisplayWidth)   at CommandLine.Parser.MakeParserResult[T](ParserResult1 parserResult, ParserSettings settings)  
at CommandLine.Parser.ParseArguments[T](IEnumerable`1 args)   at UserQuery.Main(String[] args)  
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()

@moh-hassan
Copy link
Collaborator

moh-hassan commented Mar 18, 2019

Passing Null values:

       //this cause an exception Value cannot be null. Parameter name: element
     //because 'Opt ' is required (Required = true)
       Main(new string[0])
       Main(new []{""})

Cause an exception and that exception is expected because 'Opt ' is required (Required = true)
If you passed a value:

         //this is working fine
          Main(new []{"-o"})

It will work fine.

You can catch the exception using try .. catch

I test your code using LinqPad v5.36.03 and CommandLineParser v2.4.3 (and also 2.3.0) and it's working as I described above.

This behavior is different than the Unit test, because the executor of the program is the Unit test framework and that can be workaround using that class

@franklin-ross
Copy link

franklin-ross commented Mar 19, 2019

@moh-hassan: I understand that Opt is required, I explicitly set up that example to fail parsing and trigger the help text to be printed. Is that not supposed to happen? I get this same exception with and without .WithNotParsed(errors => errors.Dump()).

Personally I did not expect an exception to be thrown just because the command line arguments were incorrect. Even if an exception was going to be thrown, I would very much expect it would have sensible and contextual help text about the option in question, and I would be monumentally surprised for it be thrown from inside a system function like System.Attribute.GetCustomAttributes.

I'm not interested in the behaviour of the "successfully parsed" path at this point, that does seem to be working fine. Just purely reporting that there seems to be a bug in the "could not parse path", and providing another environment where it seems to manifest.

It seems to me to be the same bug, as the exception message and the lower part of the stack trace is identical. Maybe it can't be worked around in the same way using that class, but that doesn't make it a different bug.

@moh-hassan
Copy link
Collaborator

@franklin-ross

I explicitly set up that example to fail parsing and trigger the help text to be printed. Is that not supposed to happen?

YES, you are correct.

I did more investigation and found that LinqPad has for every query it run a random assembly, something like:

query_syzjbp, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null

This assembly, which is considered for CommandLineParser, as the Executing assembly has no AssemblyAttributes at all.

so, the problematic TWO lines

                auto.Heading = HeadingInfo.Default;
                 auto.Copyright = CopyrightInfo.Default;

raise an exception

This why I provided a Proposal for change: Remove raising exception when Copyright or company assembly attributes are null and set with default values #403

I hope we can get a forward step to resolve this exception.

@moh-hassan
Copy link
Collaborator

This issue is resolved in the release 2.5.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants