Resolve issue with custom tests names and test Ids

[johnmwright]

This PR is to address #622

Key highlights:

• The FullyQualifiedName is based on Type+Method and independent of DisplayName.
• The TestCaseId is unique across all test cases (including data-driven ones) and deterministic between discovery sessions.

Background information:

• The VS test system expects the TestCase.FullyQualifiedName to be the actual test method name (not the display name), optionally including param types (not values). Basically, the details that would have been returned from caller.GetType().FullName
• If you don't provide a value for TestCase.Id, the class with create a GUID value based on the TestCase.FullyQualifiedName.
• When running tests, if you instruct the system to run a subset of tests (ie: not "all" tests), a filter is provided by VS that includes a list of tests by FullyQualifiiedName, which the NUnitAdapter attempt to apply in TfsTestFilter.CheckFilter(TestCase).
• The test hierarchy in the VS Test Explorer UI uses the FullyQualifiedName to determine the namespace groupings
• If multiple tests results are associated with the same TestCase instance (ie: TestCase.Id is equal), then it will show the test once in the UI, with multiple result values, such as this (which is how MSTest-based tests display results when a method is used for test cases):
  

How this issue manifested:

• The current logic sets the DisplayName and FullyQualifiedName to the display name of the test, which break the contract with VS if the user has utilized the SetName() method to override the display name value. While VS doesn't strictly enforce the FullyQualifiedName is an actual method, it does have some minor validation to make sure some special characters are used as expected, such as paired parenthesis, resulting in an error such as:

An exception occurred while invoking executor 'executor://nunit3testexecutor/': Incorrect format for TestCaseFilter Error: Missing '('. Specify the correct format and try again. Note that the incorrect format can lead to no test getting executed.

How I addressed the issue:

• First, but setting the TestCase.FullyQualfiedName to the actual test method name, this conforms with the expectations from VS and fixes the parsing issues. Since the NUnit-parsed data includes a classname and methodname attribute, the FQN can be created from that (or, at least, a close-enough version).
• By using the method type+name for the FullyQualifiedName, if the adapter doesn't explicitly set the TestCast.Id, it will result in each TestCase having the default Id based on the FQN, as described in the Background Information above. Meaning: When the adapter recreates the TestCase object for association with the TestResult object returned with the test results, it would return several results with the same TestCase.Id value, since each of those would have the same FQN. This resulted in the UI displaying the results under the same Test (similar to the screenshot above). This does not match the existing behavior, where each test case is listed as it's own node in the test case explorer. Additionally, only the first DisplayName was shown in the UI, since it ignored tests with identical Ids, due to Ids needing to be unique. Since NUnit already applies a unique id to each test, which is consistent across test runs, I decided to resolve this issue by assigning the TestCase.Id property using the NUnit id attribute value. This will give each TestCase a unique Id even if they have the same FullyQualifiedName, resulting in the individual nodes that is currently used.

Impacts of changes

I primarily used two sets of tests while working on this PR. The first is a test class I put together from the use cases listed in #622, which I put into this gist and ultimately added as a unit test in this PR. The other is the sample solution provided in comments to #622 . I'll be using these to describe the impacts of this change.

Before the changes, when the problematic parameterized tests were run, you would see:

• the actual name of the test (example: Repo_jwright.DemoTest2 in screenshot 1), in addition to the parameterized custom names (ex: Repo_jwright.Name with mismatched quote '"c') as peers.
• Tests which had mismatched special characters were represented as inconclusive (aka "Not Run") at the end of the test run, due to the "Incorrect format for TestCaseFilter" error (ex: Repo_jwright.DemoTest2 in screenshot 1)
• Parameterized tests were grouped across many nodes when custom names were given (see various nodes for ExpressionTransatorTest in screenshot 2)

Screenshot 1:

Screenshot 2:

After the changes:

• The Test name itself is no longer a node with results, but rather it's a grouping node (see: Repo_jwright.DemoTest2 in screenshot 1)
• Tests which have mismatched special characters no longer throw an error and now show their proper results
• Parameterized tests are grouped under the same node based on the FullyQualifiedTestName value (see screenshot 2)

Screenshot 1:

Possible Breaking Change

Because this PR changes how the TestCase.Id property is generated, this will result in test cases having different IDs than they did before the change was implemented. Any systems that expect the test ID value to remain the same across history will lose their tracking of test cases.

This is specifically called out in the inline comments of EqtHash.GuidFromString in the VSTest codebase, but with the references between TFS/MsftDevOps Bugs and TFS/MsftDevOps workitems. I am unsure what, if any, associations TFS/MsftDevOps places with test run results.

[johnmwright]

Note: I believe this change may fix other open issues. I will associate them to this PR as I verify them.

[johnmwright]

Note: I see the CI failures and will investigate them as soon as I can.

[johnmwright]

This "Issue" (placeholder) is related to this PR: #505

[johnjaylward]

@johnmwright This looks like a huge improvement. I love that all my tests now show under a single grouping as that's how I expected them to show in the first place. Great work, and a wonderful explanation in the PR!

It looks like some test cases may need to be updated though to match the changes:

Expected: "NUnit.Tests.FixtureWithTestCases.MethodWithParameters(9,11)"
  But was:  "NUnit.Tests.FixtureWithTestCases.MethodWithParameters"

[johnmwright]

Yeah, there's a few test failures that I missed in my local runs. I'll need some time to address them.

[johnmwright]

I've updated the logic (and my description in "How I addressed the issue:") to be more accurate and handle more cases that were failing to parse the passed-in filters from VS (such as ValueTuples in the TestCase parameter signature, which were not being properly passed-in by VS).

[johnmwright]

@johnjaylward Thanks for this! It's been fixed in my latest push.

It looks like some test cases may need to be updated though to match the changes:

Expected: "NUnit.Tests.FixtureWithTestCases.MethodWithParameters(9,11)"
  But was:  "NUnit.Tests.FixtureWithTestCases.MethodWithParameters"

[johnmwright]

Ignoring the "dump" files created when troubleshooting builds using the DumpXml utility.

[peterwald]

It looks like this will be a big improvement. 👍

The key highlights being:

1. The FQN is based on Type+Method and independent of DisplayName.
2. The TestCaseId is unique across all test cases (including data-driven ones) and deterministic between discovery sessions.

[johnmwright]

@peterwald thanks! I realized I forgot to put a TL;DR at the top of the PR description, so I borrowed your "key highlights" for that purpose :)

[OsirisTerje]

The cake script should have been updated to 3.16.0, but can do that in the next step, so that we get a pre-release in the 3.16 series

[OsirisTerje]

@johnmwright Thank you for this awesome work! I believe this PR is the best one we have ever received. It is straight to the point, and the documentation you have added is way above anything else I have seen!

Not only is the documentation comprehensive and well structured, the code simple and readable, tests have been added, but you have also taken the time to verify the resolution to a bunch of other similar issues!

Really appreciate this work!

[johnmwright]

Happy to help!