commandlineparser / commandlineparser/commandline

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

Đang mở
#403 3 bình luận 2 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
C#
Star
4.8k
Fork
478
Chỉ số merge pull request
Không có pull request nào được merge trong 30 ngày

Mô tả

**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 ](https://github.com/commandlineparser/commandline/issues/354) and [issue ](https://github.com/commandlineparser/commandline/issues/389)
and [this ](https://github.com/nunit/nunit/issues/3084) 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](https://github.com/commandlineparser/commandline/blob/master/src/CommandLine/Text/CopyrightInfo.cs#L120)

**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](https://github.com/commandlineparser/commandline/blob/master/src/CommandLine/Text/CopyrightInfo.cs) and [Headinginfo.cs](https://github.com/commandlineparser/commandline/blob/master/src/CommandLine/Text/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](https://github.com/commandlineparser/commandline/blob/master/src/CommandLine/Infrastructure/ReflectionHelper.cs#L30) 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](https://github.com/commandlineparser/commandline/blob/master/src/CommandLine/Infrastructure/ReflectionHelper.cs#L97):

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](https://docs.microsoft.com/en-us/dotnet/api/system.reflection.assembly.getcallingassembly?view=netframework-4.7.2) 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 ](https://github.com/commandlineparser/commandline/issues/389#issuecomment-457400442) and no exception is fired

Hướng dẫn đóng góp

Chưa lập chỉ mục được hướng dẫn đóng góp cho kho mã nguồn này

Hướng nghiên cứu

Bắt đầu với src/CommandLine/Text/CopyrightInfo.cs và src/CommandLine/Text/HeadingInfo.cs, sau đó kiểm tra GetExecutingOrEntryAssembly trong src/CommandLine/Infrastructure/ReflectionHelper.cs. Tái hiện vấn đề trên các target kiểm thử netcoreapp2.0 và net461/net472 được đề xuất, tránh SetAttributeOverride để việc tra cứu assembly được thực thi. Hoàn thành khi các thuộc tính null không còn gây ra ngoại lệ và toàn bộ test suite đều chạy thành công trên cả hai target.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
csharp
Lĩnh vực
cli
Loại issue
Lỗi
Độ khó
4/5
Thời gian dự kiến
3-5 ngày
Mức độ hoạt động
Đình trệ
Độ rõ ràng
Khá rõ ràng
Mức phù hợp với người mới
38/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.