Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add analyzer and code fix for Is.EqualTo usage #146

Merged
merged 3 commits into from
Aug 27, 2019
Merged

Add analyzer and code fix for Is.EqualTo usage #146

merged 3 commits into from
Aug 27, 2019

Conversation

Dreamescaper
Copy link
Member

@Dreamescaper Dreamescaper commented Jul 1, 2019 •
edited
Loading

PR adds analyzer and code fix for cases, when comparison via == or != operators or via .Equals method is used instead of Is.EqualTo or Is.Not.EqualTo constraints.

2019-07-01_17-13-15

@Dreamescaper
Copy link
Member Author

@mikkelbu
Hi! Would you be able to take a look at this one?

@mikkelbu
Copy link
Member

mikkelbu commented Aug 1, 2019

Hi @Dreamescaper
Thanks for the ping. I've been on vacation for 3 weeks - and have been busy the weeks before and after the vacation, so I did not have much time to do anything except minimal work.

Your contribution is as usual much appreciated and I hope to take a look at it tomorrow or during the weekend.

@Dreamescaper
Copy link
Member Author

@mikkelbu
Thanks for update, no rush.
Hope you had great vacation!

mikkelbu
mikkelbu reviewed Aug 7, 2019
View reviewed changes
Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to review the fixer and the two test files (also I would like to test this a little manually), but I don't have more time tonight. Here are my comments so far. It is mostly a lot of nitpick 😄.

documentation/NUnit2010.md Outdated

## Description

Using `Is.EqualTo` constraint will lead to better assertion messages in case of failure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a concluding "."

documentation/NUnit2011.md Outdated

## Description

Using Is.Not.EqualTo constraint will lead to better assertion messages in case of failure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a concluding ".".

documentation/NUnit2010.md Outdated
@@ -0,0 +1,69 @@
# NUnit2010
## Use Is.EqualTo constraint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a concluding ".".

documentation/NUnit2011.md Outdated
@@ -0,0 +1,69 @@
# NUnit2011
## Use Is.Not.EqualTo constraint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a concluding ".".

src/nunit.analyzers/Constants/EqualToConstraintUsageConstants.cs Outdated
@@ -0,0 +1,12 @@
namespace NUnit.Analyzers.Constants
{
internal static class EqualToConstraintUsageConstants
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add concluding "." to all texts below.

src/nunit.analyzers/ConstraintUsage/EqualToConstraintUsageAnalyzer.cs Outdated
equalToDescriptor, notEqualToDescriptor);


protected override void AnalyzeAssertInvocation(SyntaxNodeAnalysisContext context, InvocationExpressionSyntax assertExpression, IMethodSymbol methodSymbol)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we break this line, as it is quite long.

src/nunit.analyzers/ConstraintUsage/EqualToConstraintUsageAnalyzer.cs Outdated

if (SupportedNegativeAssertMethods.Contains(methodSymbol.Name))
{
negated = !negated;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that the effect is the same, but I would rather have true instead of !negated, so I don't have to look up to figure out what the value of negated.

src/nunit.analyzers/ConstraintUsage/EqualToConstraintUsageAnalyzer.cs Outdated
{
return;
}
else if (memberAccess.Name.Identifier.Text == NunitFrameworkConstants.NameOfIsFalse)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that memberAccess can be null.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. Why? It is checked in 'if' above

src/nunit.analyzers/ConstraintUsage/EqualToConstraintUsageAnalyzer.cs Outdated

if (shouldReport && !negated)
{
context.ReportDiagnostic(Diagnostic.Create(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be written on one line (and similarly below)

src/nunit.analyzers/ConstraintUsage/EqualToConstraintUsageAnalyzer.cs Outdated
{
shouldReport = true;
}
else if (actual.IsKind(SyntaxKind.NotEqualsExpression)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think extracting some of the expressions into variables and/or perhaps adding "()" would help a lot on the readability.

@Dreamescaper
Copy link
Member Author

Dreamescaper commented Aug 8, 2019 •
edited
Loading

@mikkelbu
Thanks! Hope you had enough time to sleep :)

Updated PR based on your review.

I'm also not sure about two separate descriptors anymore. It would require all condition analyzers (i.e. to replace bool assert with constraints, I think there might be a lot of them) to do same, which would be a bit of a pain.
I would rather use single descriptor with parameterized message. Not sure if it would work well with our documentation tests thought.

Wdyt?

@mikkelbu
Copy link
Member

mikkelbu commented Aug 8, 2019

@Dreamescaper I had time to sleep, but not enough 😄.

I have thought a bit about two separate descriptors vs. a single descriptor with parameterized message. Do I understand the problem correctly that you want to avoid the duplication in defining two almost identical descriptors. As far as I can tell we will still need to have the same logic and tests as we have now to determine which message to show, but is there more (duplication) that I overlook?

The problem is not only the documentation and the documentation tests, but also VS as it will only then show one entry as there is only one descriptor, so it will not be possible to distinguish the two "cases", e.g. to enable one and disable another, and the title must be the same (and more general).

Given the above I think I prefer to have two separate descriptors, but I feel that I might overlook something.

@Dreamescaper
Copy link
Member Author

Do I understand the problem correctly that you want to avoid the duplication in defining two almost identical descriptors.

Mostly yes. Also, similar analyzers, that could be added in future (e.g. for string methods Contains, StartsWith, EndsWith etc), would have to do same.

so it will not be possible to distinguish the two "cases", e.g. to enable one and disable another

I'd say it's actually better. I highly doubt that someone will disable Is.EqualTo analyzer, but leave Is.Not.EqualTo enabled intentionally.

Anyway, I'm fine with both ways.

@mikkelbu
Copy link
Member

Then let us go with the one analyzer approach. With respect to the parameterized message, then we don't show that in the documentation, so I don't think that would be a problem.

@Dreamescaper
Copy link
Member Author

Then let us go with the one analyzer approach.

Ok, will update PR later then.

With respect to the parameterized message, then we don't show that in the documentation, so I don't think that would be a problem.

Not sure I understand this part. Could you clarify? What we don't show in documentation?

@mikkelbu
Copy link
Member

For instance the message(format) of NUnit1003 is "There are not enough arguments provided by the TestCaseAttribute. Expected '{0}', but got '{1}'.", but this is not shown in https://github.com/nunit/nunit.analyzers/blob/master/documentation/NUnit1003.md. We only show the title and the description in the documentation as it is now (and these are fixed for the existing analyzers).

But perhaps we should also show the message(format) at some point.

@Dreamescaper
Copy link
Member Author

Updated PR to use single parameterized descriptor.

Previously I checked exact descriptorId in CodeFixProvider to understand whether expresion is negated, now I put Properties dictionary with suggested constraint to Diagnostic for this purpose.

@mikkelbu
Copy link
Member

Thanks @Dreamescaper. It looks good I'll be merging the contribution now. Sorry for the long delay, but I've not had enough time to keep up with all the NUnit work.

@mikkelbu mikkelbu merged commit d211d65 into nunit:master Aug 27, 2019
@Dreamescaper Dreamescaper deleted the isEqualToUsage branch August 27, 2019 21:40
@mikkelbu mikkelbu added this to the Release 0.2 milestone Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikkelbu mikkelbu mikkelbu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release 0.2
Development

Successfully merging this pull request may close these issues.
2 participants
@Dreamescaper @mikkelbu