microsoft
`InternalTestFailureException` has been marked as obsolete
PR #4703 marked InternalTestFailureException as obsolete, and the obsoletion message asks that if someone is using it to reach out, so... here I am! I didn't see an existing issue discussing this; apologies if I missed it.
We've been using InternalTestFailureException to represent non-assertion-related failures in tests - for example, if a helper method defined in the unit test project explicitly fails when misused. This allows a developer to distinguish between "oops, my unit test has a bug" and "oops, my production code has a bug". We also use it in some mocks where we want a test to fail if the developer does not inject an implementation for a method, but where most of the tests are expected to not call that method.
It would be possible to replace our usage of InternalTestFailureException with something else, but that would be somewhat inconvenient (since then we must either re-declare it in all our test projects, or define it in a shared package and import it as a dependency).
I tend to think that for your case, you should really be using something else other than InternalTestFailureException. It all depends on how your test projects are structured. If you already have some sort of a "test utilities" project, that could be where you add your own exception type for that purpose. Otherwise, you could have the exception declared in some file that's linked to all your test projects.
Another idea is (depending on your exact needs) to simply use Assert.Fail in those situation, and simply make the message itself an indicator on "my unit test has a bug", so basically you differentiate by the message rather than the exception type.
Would that work for you?
FYI @Evangelink if you want to add something.
I think it makes sense to have some kind of generic test exception that is not assert related (could be the base of assertion) but that would definitely involve changing InternalTestFailureException to something else.
What makes me sad is that removing the exception doesn't change what kind of failures can occur, but rather how those failures can be expressed by the framework. Having the assertion-related exceptions derive from InternalTestFailureException is an interesting idea, but I think implies assertion-related failures are a narrower case of internal test failure. I've always interpreted "internal test failure" to mean "the test failed to execute and no results are available", which is definitely not true about ex. AssertFailedException.
The problem is what was exposed on MSTest and should not have been exposed (more than half of the codebase) which is making any change extremely difficult. In general, relying on something exposed but named InternalXXX is always feeling suspicious to me but I understand why you did it given there is no other option.
@Youssef1313 I feel like this is probably better to add something like MSTestException as parent of all exceptions in MSTest including InternalTestFailureException and to keep this old exception deprecated for another major version round so we give signal this is going while providing an alternative and enough time to migrate.
Now that I understand the "internal" was meant as "internal to MSTest" and not "internal to a test", I understand why it was marked as obsolete. Rather than mark it as obsolete, could the intended usage be shifted to represent "test precondition failed" or "test initialize failed" cases, like how we've been using it? If the exception is marked as obsolete, we will need to remove our usages anyway. Or, if there is concern that changing the semantic meaning could impact customers, could a new exception type be added as part of the bigger exception changes you have described, like TestPreconditionFailedException or something?
(I don't mind replacing our usages of InternalTestFailureException, especially since now it has been clarified the "internal" was "internal to MSTest", not "internal to a test". It's just that I still think being able to describe these sorts of test failures in terms of MSTest types is valuable to the MSTest framework.)
UnitTestAssertException is currently the base of exception types we have. But:
- It's abstract
- The name won't be a good fit here.
I think we could make UnitTestAssertException inherit from MSTestException (MSTestException won't be abstract).
And when it's time to break, we remove UnitTestAssertException completely from the hierarchy, and keep MSTestException (the two steps are basically effective to renaming UnitTestAssertException to MSTestException and making it non-abstract).
@Evangelink I'm not sure though why we can't introduce MSTestException in v3 and still do the break in v4?
@Evangelink I'm not sure though why we can't introduce MSTestException in v3 and still do the break in v4?
No strong reason here.
Based on discussion with @mdrexel, I still think we need an exception that is not "assertion" related in its name. I am not sure how far to go but it seems that having:
- a base class for all MSTest exception is a good idea to simplify work on user side
- a set of exceptions for assertions (we have them already)
- an exception (or a set) for fixture issues (that would be raised by users)
@mdrexel do you have any other case than setup/cleanup exception? Would it simplify something for you to have a split between setup and cleanup exceptions? Is TestFixtureException enough? Or would it make more sense to have TestInitializationException and TestCleanupException (name is not good as it's too close to TestInit and TestCleanup attributes but you get the idea.
I'd be perfectly happy with TestFixtureException! Personally I don't think granularity like TestInitialization/TestCleanup is worth the cognitive/maintenance burden, since using them would require knowledge about whether the failure is setup vs. teardown related, and for methods which are used on both paths it could become ambiguous. At least for my use-case where the failure indicates a test that only becomes recognized as malformed at runtime, a single exception like TestFixtureException to capture all cases would work perfectly.
RE: implementation details, with the recent shift from Assert.ThrowsException to Assert.ThrowsExactly/Assert.Throws, I think most new users are going to default to Assert.Throws (not immediately recognizing it will also catch derived types). So I would be worried making the exception part of the inheritance hierarchy of ex. AssertFailedException could result in cases like Assert.Throws<TestFixtureException>(...) mistakenly hiding failed asserts in the inner delegate. (There probably shouldn't be nested assertions like that, but who knows what users will cook up...)
RE: naming, I'm not aware of any other usages of "fixture" in user-facing MSTest types. (That may just be me though.) If it's not already used elsewhere, introducing the concept of a "fixture" for just this exception makes me hesitate. I can't seem to come up with any better names though 😄
We've been using InternalTestFailureException to represent non-assertion-related failures in tests - for example, if a helper method defined in the unit test project explicitly fails when misused. This allows a developer to distinguish between "oops, my unit test has a bug" and "oops, my production code has a bug".
I am just trying to understand :) What happens when the test helper code does something wrong and throws file not found exception? Then it is a bug in test code / test setup, but it is not InternalTestFailureException, So I have to go check stack trace, and there having a common test infrastructure project helps me to see where the problem is. And I get the original intent of the error - file not found.
(Preface: the following is just conventions we're using for our code which we've found helpful, and I don't mean to imply they are a global maximum or anything)
What happens when the test helper code does something wrong and throws file not found exception?
For a scenario like you're describing, our code might look something like
public class Widget
{
public void SomethingIllegal() => throw new FileNotFoundException();
}
[TestMethod]
public void MyCoolTest()
{
Assert.ThrowsException<FileNotFoundException>(
() => Instance.SomethingIllegal());
}
private static Widget Instance => GetWidgets().First();
private static IEnumerable<Widget> GetWidgets()
{
const string dbPath = "some/path/to/something.db";
if (!File.Exists(dbPath))
{
throw new InternalTestFailureException(
"Widget database is missing. Did you follow the environment setup guide? https://link.to.some.documents.example/this-repo/widget-setup");
}
using FileStream stream = File.OpenRead(dbPath); // Could theoretically throw `FileNotFoundException`
...
return widgets;
}
The InternalTestFailureException is a precondition the helper author decided to proactively check. The message tries to guide the person running tests. If someone encounters this class of error (missing environment setup), they are probably new to the codebase, and so lack understanding about how it works. A bare FileNotFoundException only indicates what is happening, not why it's happening in the context of the tests, and so someone new is unlikely to be able to act on it.
Note that the test checks that an exception is thrown when some illegal operation is performed. If Widget already existed and a developer is adding SomethingIllegal()/MyCoolTest(), they may not even realize GetWidgets() is called or could internally throw FileNotFoundException. To avoid having a helper unexpectedly cause a test to succeed, we chose to throw InternalTestFailureException for these scenarios. Because it is part of MSTest, it should never appear in production code, and a test should never need to contain something like Assert.ThrowsException<InternalTestFailureException>.
Could it be reasonable for your scenario to throw InvalidOperationException instead? You can still have a useful message there that guides the developer that they are doing something wrong.
Substitute FileNotFoundException with InvalidOperationException in the example, and the same issue would occur. InvalidOperationException is actually the biggest offender, since it's the go-to for "the object you're trying to use is not in a valid state for the operation", and there have been cases where a developer wrote a helper that threw InvalidOperationException which concealed test failures.
For our usage, the value of InternalTestFailureException is that it
- cannot appear in production code
- (because it is declared by MSTest)
- indicates that the failure is "internal"
- (I know now that the intent was for it to represent internal failures of MSTest, but the original value to us was that we thought it meant "internal to the test")
- does not conflate "an assertion failed/was inconclusive" with "the test did not run to completion"
I think that the last bullet especially is valuable to MSTest as a whole, because it allows a developer to describe a "failure" that is due to the fixture being in a bad state. The proposed TestFixtureException captures that, and makes it clear it's not assertion-related.
(Preface: the following is just conventions we're using for our code which we've found helpful, and I don't mean to imply they are a global maximum or anything)
Nothing like that was implied from my side either. I simply wanted to understand how you find it useful.
To me it feels like a Assert.Guard, Assert.Setup (or Assert.Assert 😅 ), or something like that., which checks a test pre-condition. And throws an exception specific to that. I can see how that is useful, and I've sometimes been missing an assert that checks that my test is setup correctly, vs assert that asserts that the completed corretly.
And I can see how InternalTestFailureException might give it more meaning, that this is infra error and not test error.
Even though to me it all can be substituted by throwing the InvalidOperationException that youssef suggests, and adding text that is out of place in a normal production code and that your users recognize, like "TEST DOCTOR 👩⚕️: Widget database is missing. Did you follow the environment setup guide?"
Closing as it's low value and the exception is easily replaceable by user-defined exception type.
Will there still be a new exception type like TestFixtureException added? (Original issue was that InternalTestFailureException was marked as Obsolete, but the discussion evolved to adding a new exception.)
Otherwise (as far as I know) there is no user-accessible MSTest-defined exception that can be used to interrupt a test without implying it's Assert-related.