Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data-driven tests are not fully supported for overloaded test methods #1253

Closed
drieseng opened this issue Sep 14, 2022 · 12 comments · Fixed by #3298
Closed

Data-driven tests are not fully supported for overloaded test methods #1253

drieseng opened this issue Sep 14, 2022 · 12 comments · Fixed by #3298

Comments

@drieseng
Copy link

Description

The MSTest V2 test framework does not appear to correctly support overloaded data-driven test methods.
Only tests for the first overload are actually found and executed.

Steps to reproduce

Compile and execute the unit tests in the following code fragment:

using System.Globalization;

namespace TestProject1
{
    [TestClass]
    public class UnitTest1
    {
        [DataTestMethod]
        [DynamicData(nameof(Equal_StringAndInt32), DynamicDataSourceType.Method)]
        public void Equal(string x, int y)
        {
            Assert.AreEqual(x, y.ToString(CultureInfo.InvariantCulture));
        }

        [DataTestMethod]
        [DynamicData(nameof(Equal_Int32AndString), DynamicDataSourceType.Method)]
        public void Equal(int x, string y)
        {
            Assert.AreEqual(x.ToString(CultureInfo.InvariantCulture), y);
        }

        private static IEnumerable<object[]> Equal_StringAndInt32()
        {
            yield return new object[] {"1", 1};
            yield return new object[] {"2", 1};
        }

        private static IEnumerable<object[]> Equal_Int32AndString()
        {
            yield return new object[] {1, "0"};
            yield return new object[] {2, "2"};
        }
    }
}

Expected behavior

The test results in both Visual Studio and the console runner report 4 tests: two for the Equal(int x, string x) overload and two for the Equal(string x, int x) overload. For each of these overloads, one test should pass, and one should fail.

Actual behavior

Only the results for the Equal(int x, string x) overload are reported in Test Explorer:

image

Running dotnet test -t --nologo --no-restore --no-build also finds only two tests:

Test run for TestProject1.dll (.NETCoreApp,Version=v6.0)
The following Tests are available:
    Equal (1,1)
    Equal (2,1)

Running dotnet test --nologo --no-restore --no-build only executes two tests:

Test run for TestProject1.dll (.NETCoreApp,Version=v6.0)
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
  Failed Equal (2,1) [13 ms]
  Error Message:
   Assert.AreEqual failed. Expected:<2>. Actual:<1>.
  Stack Trace:
     at TestProject1.UnitTest1.Equal(String x, Int32 y) in C:\Users\exg906\source\repos\TestProject1\UnitTest1.cs:line 12


Failed!  - Failed:     1, Passed:     1, Skipped:     0, Total:     2, Duration: 32 ms - TestProject1.dll (net6.0)

Environment

  • Operating system: Windows 10
  • MSTest framework and adapter: v2.2.10
  • Visual Studio Enterprise 2022 (17.3.3)
  • Target framework: .NET 6.0
  • Test Execution Command Line Tool: v17.3.0
  • Microsoft.NET.Test.Sdk: v17.3.1
@Evangelink
Copy link
Member

Hi @drieseng,

Thanks for reporting this!

I have just tested and can reproduce the issue you are seeing. I have also tested with DataRow and it's also failing. I will look into the design documents to see whether this is supposed to be a supported scenario (in which case we have a bug) or if this is something we should try to support (new feature/enhancement).

@Evangelink Evangelink self-assigned this Sep 14, 2022
@Evangelink
Copy link
Member

I cannot find any mention that supports the fact this is a supported feature and testing a few older versions I cannot see it ever working so I will consider this as a feature request!

@drieseng
Copy link
Author

drieseng commented Sep 14, 2022

@Evangelink, the fact that these are silenty ignored can hardly be considered ok. Perhaps there's no documentation that indicates it is supported, but there's also no document that says it isn't.

If support this overloaded methods is added, I suppose the signature (parameter types) of the overload should be included in the name of the test method.

For example:

The following Tests are available:
    Equal(String, Int32) (1,1)
    Equal(String, Int32) (1,1)
    Equal(Int32, String) (1,0)
    Equal(Int32, String) (2,2)

To decide whether you'd want to mention the type name with or without namespace, or - when available - the C# keyword.

@Evangelink
Copy link
Member

Evangelink commented Sep 15, 2022

I understand that the behavior of silently ignoring is not ideal and should be fixed but detecting and raising some warning for this case is going to take the same time as actually implementing this request.

Besides, I am not saying we cannot support it. Actually, it's the opposite and that's why I flagged this as new feature.

Depending on the progresses we make with the scope of what needs to be done for 2.3.0, we may add it there, otherwise for sure it will be part of our v3 roadmap.

And again, thanks for reporting this problem! We are actively working on improving overall experience of MSTest users.

@drieseng
Copy link
Author

@Evangelink, if you can point me in the right direction (and if you've decided how such overloaded test methods should be reported), I can try to take a stab at this.

@Evangelink
Copy link
Member

I really appreciate that you are offering some help here! We are currently refactoring the way we produce the test ID because it is broken for datasources, I hope that he fix will either make this feature directly supported or easy to add.

Because of that, I would like to avoid having you working for nothing (or have to redo everything you did after our change). If that's ok with you, I'll ping you here once our fix is available so you can start working on it.

@Evangelink
Copy link
Member

Hey @drieseng, test ID feature is merged and I am done with the first pass of big refactorings. Let me know if you are still interested and I will try to find the place where the change would be needed.

@drieseng
Copy link
Author

drieseng commented Nov 25, 2022

@Evangelink: sure, I'd be happy to help.

@Evangelink
Copy link
Member

Awesome, I'll get back to you early next week (or during weekend) with where you could start!

@schibu007
Copy link

Are there any updates on this issue?

@Evangelink
Copy link
Member

I am not sure why I didn't get back to @drieseng so let me start by apologizing! As for the update, not really but I think that should not be too complex to add that support so I'll plan it for 3.6 (we are already loaded for 3.5).

@Evangelink Evangelink added this to the 3.6.0 milestone May 16, 2024
@nohwnd
Copy link
Member

nohwnd commented May 17, 2024

From the symptoms it looks like there will be some key on the method name, but not on methodname+parameters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants