Testfx: Backwards Compatibility: TestMethods require zero parameters

[MikeChristensen]

Description

Upon upgrading unit tests from legacy MSTest to MSTest v2, I ran into the following breaking change. TestMethods throw exceptions if there are parameters, rather than getting skipped.

Steps to reproduce

Repro code:

    [TestClass]
    public class UnitTests
    {
        [TestMethod]
        public void Works()
        {
            Assert.IsTrue(true);
        }

        [TestMethod]
        public void Breaks(string name)
        {
            Assert.IsTrue(true);
        }

        [TestMethod]
        public void AlsoBreaks(bool flag = false)
        {
            Assert.IsTrue(true);
        }
    }

Expected behavior

Legacy MSTest would ignore these methods completely. No errors or exceptions and they wouldn't get run. It would be somewhat useful if "AlsoBreaks" would get run using the default parameter values.

Actual behavior

Unit test framework throws a cryptic exception: "Parameter count mismatch".

Environment

Version .18 of MSTest on VS2015

[AbhitejJohn]

Thanks for reporting this @MikeChristensen . The message clearly needs to change. This was what the legacy framework reported:

Message: UTA007: Method Breaks defined in class UnitTestProject2.UnitTest1 does not have correct signature. Test method marked with the [TestMethod] attribute must be non-static, public, does not return a value and should not take any parameter. for example: public void Test.Class1.Test().  Additionally, return-type must be Task if you are running async unit tests. Example: public async Task Test.Class1.Test2().

As to running the tests with default parameter values, it would be best to use the TestMethodAttribute extension point to get that working. If not, this would cause a perf impact for all test runs.

[MikeChristensen]

Cool! I was meaning to check out the extension points to see what could be done with those, but haven't gotten around to it yet.

Invalid method signatures with the old framework seem to silently fail for me, at least on our build system which is running them through TeamCity. I agree in any case the error message should be as descriptive as possible.

[ShreyasRmsft]

We need to check if methodinfo has params for a dd test if not throw the same exception message legacy framework used to throw

[jayaranigarg]

@MikeChristensen :

        [TestMethod]
        public void Breaks(string name)
        {
            Assert.IsTrue(true);
        }

We believe the only scenario user would write such a test where test method takes parameters would be when he wants to pass some data to the test and want to run as a data-driven test. But since no data is provided by the user, the only possible explanation to that would be that the user actually forgot to specify the data due to some reason. So rather than skipping the discovery(behavior of MSTestv1) where User may think tests are running successfully, it would be better if we flash a error message informing him that he missed specifying parameters(behavior of MSTestV2). Keeping the current behavior of MSTestV2 as it is makes more sense to us. Please share your thoughts in this?

[MikeChristensen]

Yup, I think an error message is perfectly fine as long as that error message is descriptive and helpful.

[pvlakshm]

How about this as the error message:
Only data driven test methods can have parameters. Did you intend to use [DataRow] or [DynamicData]?