microsoft
Consider enforcing MapInconclusiveToFailed in MTP
Summary
Consider enabling MapInconclusiveToFailed in Microsoft.Testing.Platform. Possibly other currently-disabled test-quality related settings can be set to the stricter mode.
Background and Motivation
MTP can be an opportunity to let the test owners remove inconclusive test results. For code quality tests should either succeed or its results treated as non-success.
Alternative Designs
Currently this (and other) setting can be set manually.
I have some doubts on this one. I see many use cases where people use Assert.Inconclusive as some kind of dynamic skip/ignore and they do expect the test to be green.
I don't really like the Inconclusive meaning because it's confusing to me. Maybe it would be good to deprecate this API, turn this on by default. I don't know if we should expect from users they just return for the "skip" or introduce some kind of Assert.Skipped
I agree with @Evangelink. Changing the default of this flag is too breaking, and will be confusing because now the "default" behavior makes it that Assert.Fail and Assert.Inconclusive are the same.
To me, the ideal world is renaming Inconclusive to Skip, and not having this feature flag at all. But that as well is too breaking probably.
I think if a repo wants to forbid usage of Inconclusive, it should be done via BannedApiAnalyzers.
We are unlikely to touch this in v4.
Looking at the usages of inconclusive: https://grep.app/search?f.lang=C%23&q=.Inconclusive it is often used in place of the other conditional attributes we now have, or as a way to bail out from a test early when it fails some pre-condition, e.g. failed to create worksheet, or timed out, or is not implemented yet.
There seem to be much better ways to express those with skip / actually failing / explicit tests. I haven't yet found one where it would really make sense to me to use Inconclusive.
So to me MapInconclusiveToFailed is like warnAsError, something that you can use locally to make tests pass on your dev system but should probably fail in CI. But with it being false by default we don't fail in CI, and users keep not running their tests.
We also map inconclusive to skipped, which is in many cases "hiding" an error. So to me it would make sense to deprecate the Inconclusive state altogether, and set MapInconclusiveToFailed=true, with suggestions on how to move to skip / fail / conditional / explicit tests.
Could we keep as-is for now and build some analyzers to suggest different APIs and do break in v5?
@nohwnd We have a use case for Assert.Inconclusive in this project: It's used both in the way you described, to skip running a test when a precondition isn't met (like if a dev forgot to download the supplementary files for the test suite), but also as a third end state, distinct from a pass.
