TestContext.TestName in data-driven tests changed behavior in 2.1.2

[majastrz]

Description

After upgrade to MSTest.TestAdapter version 2.2.1 from 2.1.2, our [DataTestMethod] tests changed behavior. TestContext.TestName now contains a badly serialized representation of the data row being passed in.

We are using test names to construct result file paths. This change breaks them on Windows due to Windows file system rules. The package didn't have a major version rev, so this behavior change is unexpected.

If this one intentional, how do we get the old behavior?

Steps to reproduce

What steps can reproduce the defect?

using Microsoft.VisualStudio.TestTools.UnitTesting;
using System;

namespace TestProject { [TestClass] public class UnitTest1 { public TestContext TestContext { get; set; }

    [TestMethod]
    public void TestMethod1()
    {
    }

    [DataTestMethod]
    [DataRow("hello", "there")]
    [DataRow("general", "kenobi")]
    public void Foo(string one, string two)
    {
        Assert.Fail($"{this.TestContext.TestName} - {one} - {two}");
    }
}

}

## Expected behavior
On 2.1.2, we see the following output from `dotnet test`:

Starting test execution, please wait... A total of 1 test files matched the specified pattern. Failed Foo [36 ms] Failed Foo (hello,there) [33 ms] Error Message: Assert.Fail failed. Foo - hello - there Stack Trace: at TestProject.UnitTest1.Foo(String one, String two) in C:\Users\majastrz\source\repos\SerializedGarbage\TestProject\UnitTest1.cs:line 21

Failed Foo (general,kenobi) [< 1 ms] Error Message: Assert.Fail failed. Foo - general - kenobi Stack Trace: at TestProject.UnitTest1.Foo(String one, String two) in C:\Users\majastrz\source\repos\SerializedGarbage\TestProject\UnitTest1.cs:line 21

Failed! - Failed: 2, Passed: 1, Skipped: 0, Total: 3, Duration: 37 ms - TestProject.dll (netcoreapp3.1)

The relevant part is this: `Assert.Fail failed. Foo - hello - there`

## Actual behavior
On 2.2.1, we see the following output from `dotnet test`:

Starting test execution, please wait... A total of 1 test files matched the specified pattern. Failed Foo [100 ms] Failed Foo (hello,there) [97 ms] Error Message: Assert.Fail failed. Foo(System.String,System.String) - hello - there Stack Trace: at TestProject.UnitTest1.Foo(String one, String two) in C:\Users\majastrz\source\repos\SerializedGarbage\TestProject\UnitTest1.cs:line 21

Failed Foo (general,kenobi) [< 1 ms] Error Message: Assert.Fail failed. Foo(System.String,System.String) - general - kenobi Stack Trace: at TestProject.UnitTest1.Foo(String one, String two) in C:\Users\majastrz\source\repos\SerializedGarbage\TestProject\UnitTest1.cs:line 21

Failed! - Failed: 2, Passed: 1, Skipped: 0, Total: 3, Duration: 101 ms - TestProject.dll (netcoreapp3.1)

The relevant part is this: `Assert.Fail failed. Foo(System.String,System.String) - hello - there`

## Environment
Repro project file:
```msbuild
<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>netcoreapp3.1</TargetFramework>

    <IsPackable>false</IsPackable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.9.1" />
    <PackageReference Include="MSTest.TestAdapter" Version="2.1.2" />
    <PackageReference Include="MSTest.TestFramework" Version="2.1.2" />
    <PackageReference Include="coverlet.collector" Version="3.0.3">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
  </ItemGroup>

</Project>

The behavior in VS after the upgrade is also strange. TestExplorer chokes on the project after upgrade and refuses to run tests. However, this is reproducible via dotnet test outside of VS as well.

AB#1290016

[majastrz]

Linked the dependabot PR that led to the discovery of this: https://github.com/Azure/bicep/pull/1673

[sumitkm]

@majastrz are you able to run any tests with [DataTestMethod] from Visual Studio? I can't even run the tests from Visual Studio... VS simply creates a new Branch of Tests not executed. The only message is

No test matches the given testcase filter `FullyQualifiedName=Tests.Unit.Common.Utilities.MyTestClass.TestMethod1`

I have tried the clean, restart vs, rebuild to no avail. Going to try and run this from command line and see if is any good.

Someone let me know if I should file this as a separate bug or is this related to the one filed by @majastrz

[majastrz]

Yeah on a new project I wasn't able to run any tests with [DataTestMethod]. Downgrading MSTest.TestAdapter fixed it. On an old project, they somehow worked but had the issue mentioned above.

I think this repo has different rules for submitting bugs in VS. I'd like to keep this issue specific to the TestName behavior change.

[Haplois]

Thank you for the report. I have determined the root cause of the issue. A preview package with the fix will be available later today or tomorrow morning. When it's available, I'll update the issue.

[Haplois]

Two more cases of this same issue: https://github.com/microsoft/testfx/issues/789#issuecomment-793565132, https://github.com/microsoft/testfx/issues/789#issuecomment-792730365

[Haplois]

This is fixed in 2.2.2-preview-20210310-02. The preview package can be found here: https://dev.azure.com/dnceng/public/_packaging?_a=feed&feed=test-tools

[Haplois]

The version v2.2.2 release with the fix.

[iesmat]

@Haplois I've reproduced this in the latest MSTest.TestAdapter 3.0.0. Is there a separate issue in 3.0.0 because it looks like your change is in rel 3.0.0? We are using the Visual Studio Test ADO Pipeline Task and get the following when we enable re-run failed tests (test names replaced for confidentiality purposes):

##[error]Incorrect format for TestCaseFilter Missing Operator '|' or '&'. Specify the correct format and try again. Note that the incorrect format can lead to no test getting executed. No test matches the given testcase filter `FullyQualifiedName=Namespace.TestName|FullyQualifiedName=Namespace.TestName2 (Input)|FullyQualifiedName=Namespace.TestName3 (Input)' in ...

[Haplois]

If you share your logs with @Evangelink over teams I am pretty sure he'll help you. It looks like there is either a bug with our parser or the formatting of the filter provided. If you share the entire filter him, he'll be able to help you.

I am no longer with Microsoft, but my twitter DMs are open, if you prefer I can look into it too. Just send me a message with exact error.

@iesmat

[iesmat]

Thanks

[Evangelink]

Hi @iesmat, would you mind opening a new issue with a reproducer so we can investigate it?

[Evangelink]

Hi there, if the issue is about having the parameters type as part of the test name, it seems to be by design as far as I can tell from the code so I don't think there is any bug. It's a change of behaviour because there was a bug in the implementation and yes MSTest wasn't respecting the semantic versioning but we are trying to do so starting with v3.

[iesmat]

Hi @Evangelink , We are using the Visual Studio Test (2.*) ADO pipeline Task to run our tests. This is the one that doesn't like the new format. If this is by design with the new v3, then how do we get our ADO pipeline task to handle this new format? We already have it using the Latest test platform version with re-run enabled:

[Evangelink]

This is not a change done in v3, it's the case since 2.2.4. We are not owning the AzDo task, so I would recommend you to reach that team/tool and raise an issue with them.

[iesmat]

I don't think it's the AzDo task issue though because when i switch versions of the mstest adapter in the project, it either works or doesn't work. The issue is that the mstest adapter is specifying a TestCaseFilter with parenthesis:

[error]Incorrect format for TestCaseFilter Missing Operator '|' or '&'. Specify the correct format and try again. Note that the incorrect format can lead to no test getting executed.

No test matches the given testcase filter `FullyQualifiedName=Namespace.TestName|FullyQualifiedName=Namespace.TestName2 (Input)|FullyQualifiedName=Namespace.TestName3 (Input)' in

From the other issue linked in this bug, you need to escape the parenthesis or not provide the DataRow input in the TestCaseFilter.

https://github.com/microsoft/vstest/issues/2807

[iesmat]

or are you implying that the ADO task is the one that should escape the TestCaseFilter string?

[Evangelink]

I am quite confused... This issue is about TestName, your screenshot is about AzDo retry feature and now you are talking about test filter.... There seems to be different topics. If your issue is about TestName, as far as I can see in the code it seems to be by-design.

If you have a problem with the retry capability, this is handled by AzDo team and has nothing to do with MSTest.

If the issue is about test case filtering, as suggested above, can you please open an issue and add some minimal repro to investigate?

[iesmat]

That's what I'm trying to find out I guess. It seems that the fix to the problem was to revert the name changes that was done:

// This causes compatibility problems with older runners.
// return string.IsNullOrWhiteSpace(this.TestMethod.ManagedMethodName)
//      ? this.TestMethod.Name 

Even now the display name code still has above code, but other code has been added to the ToTestCase() function to append some more information.

Basically, MSTest.TestAdapter version 2.1.0 had this output: vstest.console.exe /TestCaseFilter:"FullyQualifiedName=Namespace.TestName"

Whereas, MSTest.TestAdapter version 3.0.0 had this output: vstest.console.exe /TestCaseFilter:"FullyQualifiedName=Namespace.TestName (input)"

Which causes the error "##[error]Incorrect format for TestCaseFilter Missing Operator '|' or '&'. Specify the correct format and try again. Note that the incorrect format can lead to no test getting executed."

So i'm guessing that the failed test function name being returned to the ADO Task now has (input) which is causing it to fail.

So is this an ADO Task issue where they now have to escape the parenthesis? For example, vstest.console.exe /TestCaseFilter:"FullyQualifiedName=Namespace.TestName \(input)\"