Deprecate ExpectedException attribute.

[jimontheriver]

Description

I can still write tests that use ExpectedExceptionAttribute.

Steps to reproduce

Write this test
[TestMethod]
[ExpectedException(typeof(ArgumentException))]
public void TestMyStuff()
{
// arrange
var instanceUnderTest = new YourFunkyClass();
DoSomethingThatThrowsArgumentException();

// act // assert
Assert.ThrowsException(()=>instanceUnderTest.DoStuff("badargument");}
}

Expected behavior

I understand backwards compatibility, but ExpectedException is WIDELY recognized to be a source of tests passing when they should be failing. Any test with ExpectedException is effectively useless unless there are absolutely no other calls in the test. You just don't know if it's failing for the reason you expect it to fail, or if it's even getting to the line you want it to fail on.

At a minimum, there needs to be a way to set the test runner to reject it so that if someone brings it in, it fails. By MSTest 3, the attribute should go away entirely.

Actual behavior

I can still use it.

Environment

plain Win10, VS2017.

AB#1934068

[AbhitejJohn]

@jimontheriver I agree. ExpectedException would probably be tagged with an obsolete tag in a future release. So building a project with an ExpectedException would start throwing warnings.
/cc @pvlakshm.

[AbhitejJohn]

We would need to fix the intellitest wizard that outputs ExpectedException in-tandem with this change so as not to break that workflow.

[ObsidianPhoenix]

I was actually thinking it would be better to extend it so that you can be more specific with the exception. i.e. permit specifying the error message (and/or the paramName). The attribute can save a lot of boilerplate code in tests when you are testing multiple failure paths.

I agree though that, as is, its not as accurate as it could be. For instance, consider this code:

public void DoWork(Person person)
{
    if (person == null) throw new ArgumentNullException(nameof(person));
    if (string.IsNullOrWhiteSpace(person.Forename)) throw new ArgumentException($"{nameof(person.Forename)} should be specified", nameof(person));
    if (string.IsNullOrWhiteSpace(person.Surname)) throw new ArgumentException($"{nameof(person.Surname)} should be specified", nameof(person));
    if (string.IsNullOrWhiteSpace(person.Email)) throw new ArgumentException($"{nameof(person.Email)} should be specified", nameof(person));
}

[TestMethod, ExpectedException(typeof(ArgumentException))]
public void WhenSurnameIsNull_ExceptionThrown()
{
    var person = new Person() { Forename = "Test" };
    DoWork(person);
}

As is, there is no guarantee that the exception is being thrown on the property I'm intending. If someone inadvertently swaps the checks around, or adds a new property, this test could still pass even though its not on the correct check.

To do this without the attribute requires something like this:

[TestMethod, ExpectedException(typeof(ArgumentException))]
public void WhenSurnameIsNull_ExceptionThrown()
{
    var person = new Person() { Forename = "Test" };
        try
        {
            DoWork(person);
        }
        catch (ArgumentException ex)
        {
            Assert.IsTrue(ex.ParamName == nameof(Person.Surname)
                && ex.Message == $"{nameof(Person.Surname)} should be specified");
        }
}

But by extending the attribute, you could do this:

[TestMethod] 
[ExpectedException(typeof(ArgumentException), ParamName := "person", $"{nameof(Person.Surname)} should be specified")]
public void WhenSurnameIsNull_ExceptionThrown()
{
    var person = new Person() { Forename = "Test" };
    DoWork(person);
}

This lets developers be much more specific with their Exception Testing. Overloading the constructors will allow this enhanced behaviour without breaking existing tests.

[robdalsanto]

We make use of this attribute, so please keep it. That you can specify a specific exception type is meeting our requirements for avoiding false 'test pass' situations.
Without this, we would wind up writing our own try-catch-verify helpers.
--- edit Sept 18, 2108 ---
Thanks @ziomyslaw, I failed to notice the Assert.ThrowsException. Will plan to move to that.

[ziomyslaw]

@robdalsanto you can specify a specific exception type

Assert.ThrowsException<T>(Action action)
Assert.ThrowsExceptionAsync<T>(Func<Task> action)

[Evangelink]

I suggest we make the attribute obsolete and provide a code refactoring to automatically rewrite the code to use new/better APIs.

[fforjan]

@Evangelink do you already have the code analyser in this repository for the refactoring tool ? I was looking for it but could not found it ?

[Evangelink]

@fforjan Sadly there are none but I really want to add them! I was expecting to be working on that early this year but priorities have changed

[Evangelink]

We will deprecate the attribute as part of 3.3.0, the analyzer is added as part of 3.2.0.

[Evangelink]

After a couple of internal discussions, we want to try to be more strictly following semantic versioning as we have some users with huge test suite and this kind of change would lead to many failures and some time to fix or ignore. Pushing back to v4 as it will allow breaking changes.

[Kissaki]

@Evangelink you self-assigned this ticket in December. It still has the Help-Wanted label. I assume that label is no longer applicable and should be removed?

[Evangelink]

Hi @Kissaki,

I wanted to release v4 a long time ago but we decided to postpone its release so we can group more breaking changes to avoid doing many breaks increments.

We have already provided the analyzer so we only need to drop the attribute in v4.