Test maintainance: add missed tests and removing xUnit1013 warning

[moh-hassan]

• Add missed tests in v2.4.3 which were reported by @ericnewton76 (18 test case)

• Remove the xunit test warning xUnit1013:

warning xUnit1013: Public method 'XXX' on test class 'YYY' should be marked as a Fact.

• Fix Xunit warning: Skipping test case with duplicate ID

• Fix Xunit warning: xUnit1014- MemberData should use nameof operator to reference member

[NeilMacMullen]

I notice you have fixed the test by changing the copyright check to ...

lines[0].Should().StartWithEquivalent("testhost");
lines[1].Should().StartWithEquivalent("© Microsoft Corporation");

This is probably correctly for the build server but unfortunately still fails on my local machine because I use Ncrunch (continuous test runner) and so the assumption about the name of the test-runner is incorrect. I would suggest simply not testing the copyright lines OR raising a feature request to ensure that the lines are consistent with the .net framework build.

[moh-hassan]

NCrunch is a good testing suit but it's commercial (not free).
dotnet test suit is free and available in both local machine and CI servers (windows/Linux and OS X) and so simplify the fix of test between the Contributors and unify test result report.

I can modify the PR and set these copyright values as a constant in a separate class so you can replace your constant values during local test.

All I did in my local machine is run the command:

       dotnet   test

Is there any constraint in your development Environment of using dotnet test with Xunit?

[NeilMacMullen]

My point is more that you really don't want to discourage contributors from running local unit tests and hard-coding an assumption on the test-runner is bad practice for plenty of reasons - not least of which you might one day change your build system and some future maintainer will then by mystified by the fact that half the unit-tests break ;-) A unit test should really not be relying on an 'external' variable such as the name of the calling process.

[moh-hassan]

I agree with you that unit test should really not be relying on an 'external' variable.
The problem is that the majority of test cases depend on some hard-wired values that is based on test suit runners.
I will investigate of excluding these copyright value test and modify my PR.

You can exclude these copyright tests from the test case and continue using NCrunch .

[NeilMacMullen]

Here is a better fix (also better than my local changes)… Bearing in mind the intention of the test is to establish that the help-text contains the copyright info you can use

lines[1].Should().Contain(CopyrightInfo.Default.ToString());

which works for all platforms and test runners!
I'll post the equivalent formulation for lines[0] when I find it...

[NeilMacMullen]

Ok- here are the two lines you want to use. These are platform independent so you can get rid of the #if check....

            lines[0].Should().Be(HeadingInfo.Default.ToString());
            lines[1].Should().Be(CopyrightInfo.Default.ToString());

Establishing that the ReflectionHelper (which is used by HeadingInfo and CopyrightInfo) does correctly pull attributes from the assembly is a separate concern and should be tested separately.

[moh-hassan]

Good solution.
I'll modify my PR and use your solution :).

[moh-hassan]

@NeilMacMullen
Modification based on your solution is done in this PR,(a939e1e), and test pass successfully in CI appveyor.

[NeilMacMullen]

hooray! :-) Are you planning to merge this back to develop? If so I will integrate it with my PR.

[moh-hassan]

Yes, I plan to merge it with other PRs including your PR in develop.
You can do (if needed) an integration test with it in a throw-away branch to be sure that all tests works in your PR. No need you merge it into yours. This simplify the last minute maintenance of PR(s).