Make diagnostic's severity consistent

[mikkelbu]

The following is extracted from #292 (comment) and I agree with the suggestions

We seem to be inconsistent in severity, especially the Classic analyzers (which I added) some are warning, some info.
My suggestions are:

• Any code that is correct, like the Classic analyzer, should be info by default.
• Any code that has maintainability issues, such as use nameof, should be a warning
• Any code that is wrong, such as incompatible types, should be error.

[mikkelbu]

I went through all the current diagnostics, and the following is my questions/comments (but I very much welcome input and comments):

• Should NUnit1028 - "The non-test method is public." be info instead of warning?
• The following should be errors and not just warnings:
  • NUnit2020 - "Incompatible types for SameAs constraint."
  • NUnit2021 - "Incompatible types for EqualTo constraint."
  • NUnit2022 - "Missing property required for constraint.
  • NUnit2023 - "Invalid NullConstraint usage."
  • NUnit2024 - "Wrong actual type used with String Constraint."
  • NUnit2025 - "Wrong actual type used with ContainsConstraint."
  • NUnit2026 - "Wrong actual type used with the SomeItemsConstraint with EqualConstraint."
• The following should be warnings and not just info
  • NUnit2038 - "Consider using Assert.That(actual, Is.InstanceOf(expected)) instead of Assert.IsInstanceOf(expected, actual)."
  • NUnit2039 - "Consider using Assert.That(actual, Is.Not.InstanceOf(expected)) instead of Assert.IsNotInstanceOf(expected, actual)."
• NUnit2041 - "Incompatible types for comparison constraint." should be an error.

[manfred-brands]

• Should NUnit1028 - "The non-test method is public." be info instead of warning?

Inside the nunit-analyzer code base there was a public method which was supposed to be a test, but did not have an attribute.
This therefore would fall into the category of potentially wrong. On the other hand if the method was not a test method with a forgotten attribute it would be fine. The default for this rule is disabled in order not to confuse developers that use public non-test methods. For developers that want the rule, they will configure it to an error.
I suggest to enable it by default and then lower the level to info. This will try to steer developers into not adding public method to a fixture which should be self contained.

Agree with NUnit2020 .. NUnit2026 to be error.

The following should be warnings and not just info

• NUnit2038 - "Consider using Assert.That(actual, Is.InstanceOf(expected)) instead of Assert.IsInstanceOf(expected, actual)."
• NUnit2039 - "Consider using Assert.That(actual, Is.Not.InstanceOf(expected)) instead of Assert.IsNotInstanceOf(expected, actual)."

Why is that? Is the classic code not correct?

I would think that the following should be info instead of warning.

• NUnit2027 = "Consider using Assert.That(actual, Is.GreaterThan(expected)) instead of Assert.Greater(actual, expected)."
• NUnit2028 = "Consider using Assert.That(actual, Is.GreaterThanOrEqualTo(expected)) instead of Assert.GreaterOrEqual(actual, expected)."
• NUnit2029 = "Consider using Assert.That(actual, Is.LessThan(expected)) instead of Assert.Less(actual, expected)."
• NUnit2030 = "Consider using Assert.That(actual, Is.LessThanOrEqualTo(expected)) instead of Assert.LessOrEqual(actual, expected)."
• NUnit2031 = "Consider using Assert.That(actual, Is.LessThanOrEqualTo(expected)) instead of Assert.LessOrEqual(actual, expected)."

Any rule with a message that starts with "Consider".
Unless NUnit is thinking of dropping classic support for NUnit 4.

[mikkelbu]

Currently, there is no plans of dropping classic support, even though we primarily - perhaps only - add functionality to the constraint variant.

Regarding NUnit2038-NUnit2039 and NUnit2027-NUnit2031 then my reason for making these into warnings is to be consistent with NUnit2005 and NUnit2006 (avoiding the usage of Assert.AreEqual(expected, actual) and Assert.AreNotEqual(expected, actual) as people often swap expected and actual). So my thinking was that every classic assert with two arguments that could be confused should be a warning, but probably this mostly happen for equality comparisons, so perhaps we should just limit the warnings to NUnit2005 and NUnit2006.

[manfred-brands]

Yes, I have seen several people swap actual and expected in the AreEqual variants. So having these are warning is a good thing.
Then change all other classic ones to suggestions.