Brightspace / D2L.CodeStyle

Annotations and analysis tools for D2L C# code style
Apache License 2.0
10 stars 22 forks source link

NUnit Assert.IsTrue/IsFalse alternatives analyzer #346

Open j3parker opened 6 years ago

j3parker commented 6 years ago

Assert.IsTrue and Assert.IsFalse are often misused. Alternative assertions offer better information when the test fails. For example, with classic model assertions:

- Assert.IsTrue( x < y );
+ Assert.Less( x, y );
- Assert.IsFalse( z == null );
+ Assert.NotNull( z );

or with the fluent APIs (reference):

- Assert.IsTrue( x < y );
+ Assert.That( x, Is.LessThan( y ) );
- Assert.IsFalse( x == null );
+ Assert.That( z, Is.Not.Null );

we can detect most bad IsTrue/IsFalse assertions by checking if the first argument is a BinaryExpressionSyntax. syntax.OperatorToken could make to the correct assertion/constraint to use and syntax.Left and syntax.Right can be moved around to arguments of the new assert (and constraint if we went that way.)

Open questions

Classic model assertions or fluent assertions?

We mostly still use classic-style I believe but maybe we should make the fixit suggest the new stuff. Do we want to adopt the new kind or not?

When the argument isn't a BinaryExpressionSyntax

This is a slightly harder case:

var result = x < y;
Assert.IsTrue( result );

is it worth catching this case? Definitely better to ignore it on the first-pass. We could do a survey to see how frequently this syntax pattern happens.

j3parker commented 6 years ago

@cbortos-d2l is working on this in his spare time and has something mostly working!

cbortos-d2l commented 6 years ago

PR for the analyzer + code fix: https://github.com/Brightspace/D2L.CodeStyle/pull/351