Proposal for change: Remove raising exception when Copyright or company assembly attributes are null and set with default values

[moh-hassan]

Problem Test fail in Xunit or Nunit framework in net4x when Copyright or Company assembly attributes are null

Problem Analysis The problem is existing since version 2.3 and now in v2.4.3 but the behavior is different based on the test project is net4x. The test fail for the test cases that test --help or --version or even errors for bad options in the commandline See this issue and issue and this in NUnit project

These issues can't be detected by the current unit test project because unit test uses the method SetAttributeOverride' and bypass the EntryAssemply`

The CommandLineParser in Production is running Ok when reading AssemblyAttributes copyright and company, but in Test project in Full Framework net4x or latter it cause the failure of test when reading assembly attributes. The test is OK in netcoreapp2.x

In production the entryAssemply is the console project itself which have copyright and company assembly attributes but in test , the executer is the testhost framework itself which has null values for copyright and company assemply attributes.

This because the CommandlineParser raise Exception when CopyRight/Company are null see this

Behavior in v2.3 in net45x or latter: Assembly.GetExecutingAssembly(); in netstandard1.5 or latter: Assembly.GetEntryAssembly()

	 private static Assembly GetExecutingOrEntryAssembly()
			{
				var assembly = Assembly.GetEntryAssembly();

	#if !NETSTANDARD1_5
				assembly = assembly ?? Assembly.GetExecutingAssembly();
	#endif

				return assembly;
			}

Behavior in v2.4.3 in netstandrad2.x :Assembly.GetEntryAssembly();

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

Advantage of removing raising exception

• The unit test don't fail
• No side effect at all for the flow of the CommandLineParser.
• Even, in test, the values of copyright and company is not valid in unit test and they are the values in hosttest assemblies and currently CommandlineParser pass artificial values for them in unit test.
• These values are different based on the version of testhost used and causes troubles to the unit test.
• Setting default values is better than firing exception.

Suggested Solution PR can modify CopyrightInfo.cs and Headinginfo.cs to remove exception and set default values.

1. When reading copyright and it's null , don't fire exception and set a default value like "copyright @currentYear"
2. When reading Company and it's null , don't fire exception and set a default value like "Copmpany @productName"
3. Modify the method GetExecutingOrEntryAssembly as described below

Consideration for Unit Test

• Test project is converted to multi-target project netcoreapp2.0 and net461
• Avoid using the method SetAttributeOverride because it bypass the the method GetExecutingOrEntryAssembly which is under test.

Proof of Concept POC can be done for evaluating this change.

Update Also, the following code in ReflectionHelper:

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

can be modified to:

                       private static Assembly GetExecutingOrEntryAssembly()
			{
                                // this insures that assembly will not be null
                                  var assembly = Assembly.GetEntryAssembly() ?? Assembly.GetCallingAssembly()        
				return assembly 
	                 }

Assembly.GetCallingAssembly Returns the Assembly of the method that invoked the currently executing method

POC I did the modification of GetExecutingOrEntryAssembly as described in (GetExecutingOrEntryAssembly) and modified the test project to multi-target netcoreapp2.0 and net461/net472 and all tests (334 test case) pass successfully in both targets. Also I test the issue 389 and no exception is fired

[ericnewton76]

Great write up of the problem. Thanks.

Some of these changes revert Viktor's netstandard PR, which is fine... I was never really crazy about just targeting netstandard2.0 given some of the other support for net40 and net45 we've had traditionally.

[moh-hassan]

Thanks @ericnewton76 I appreciate your support to this change and happy to cooperate with you. V2.4.3 is valuable to support the new era netcoreapp2.x and my PR for support net45,net40 is ready with very minimum changes. If you agree I can prepare for a new PR and I I'll take into account adding the 16 unit test that are excluded in v2.4.3. Let me know what is needed for the next step. Best regards,

[mnivet]

Hi, I've read all the issue and previous ones related to this, and I wonder if instead of just trying to make unit tests not raising exceptions, it would not be better to allow that even those unit tests scenarios correctly read all the attributes from the right assembly like when running in Production ?

And for that I wonder if it would be possible to explicitly give to CommandLine.Parser class (probably through ParserSettings) the assembly in which the attributes should be read instead of trying to 'guess' it with methods like GetExecutingOrEntryAssembly()...

I've not deep dive in the code of this project but as a consumer I would found reasonable to write something like:

    public static class Program
    {
        public static int Main(string[] args)
        {
            var parser = new Parser(settings => settings.EntryAssembly = typeof(Program).Assembly);
            [...]
        }
    }

Like that the entry assembly could be defined by the consumer and this should avoid magical variations between unit tests and production executions, or netframework vs netcoreapp executions, or what else could come later.