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.

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

[java] JUnitTestsShouldIncludeAssert: Recognize AssertJ soft assertions as valid assert statements #1434

Merged
merged 9 commits into from Nov 15, 2018

Conversation

Projects
None yet
4 participants
@ledoyen
Copy link
Contributor

ledoyen commented Nov 5, 2018

The JUnitTestsShouldIncludeAssertRule was missing AssertJ soft assertions.

Soft assertions are used this way:

 @Test
 public void host_dinner_party_where_nobody_dies() {
   Mansion mansion = new Mansion();
   mansion.hostPotentiallyMurderousDinnerParty();
   SoftAssertions softly = new SoftAssertions();
   softly.assertThat(mansion.guests()).as("Living Guests").isEqualTo(7);
   softly.assertThat(mansion.kitchen()).as("Kitchen").isEqualTo("clean");
   softly.assertThat(mansion.library()).as("Library").isEqualTo("clean");
   softly.assertThat(mansion.revolverAmmo()).as("Revolver Ammo").isEqualTo(6);
   softly.assertThat(mansion.candlestick()).as("Candlestick").isEqualTo("pristine");
   softly.assertThat(mansion.colonel()).as("Colonel").isEqualTo("well kempt");
   softly.assertThat(mansion.professor()).as("Professor").isEqualTo("well kempt");
   softly.assertAll();
 }

This PR is about detecting a call to SoftAssertions#assertAll as a valid assert statement.

As a bonus, this also works with Java 10+ syntax using the var keyword if the type can be inferred with the second member of the declaration statement.

Fixes #1435

@ledoyen ledoyen changed the title Recognize AssertJ soft assertions as valid assert [java] [junit] Recognize AssertJ soft assertions as valid assert statements Nov 5, 2018

@pmd-test

This comment has been minimized.

Copy link

pmd-test commented Nov 5, 2018

1 Message
📖 This changeset introduces 0 new violations and 0 new errors,
removes 0 violations and 0 errors. Full report
This changeset introduces 0 new violations and 0 new errors,
removes 255 violations and 16 errors. Full report
This changeset introduces 0 new violations and 0 new errors,
removes 255 violations and 16 errors. Full report
This changeset introduces 0 new violations and 0 new errors,
removes 0 violations and 0 errors. Full report
This changeset introduces 3 new violations and 0 new errors,
removes 0 violations and 0 errors. Full report

Generated by 🚫 Danger

@ledoyen ledoyen force-pushed the ledoyen:feature/recognize_assertj_soft_asserts_as_assert branch 2 times, most recently from fc076d3 to e3e8667 Nov 5, 2018

@adangel
Copy link
Member

adangel left a comment

Thanks for your PR!

I have a few additional comments:

  • Please try to avoid making unrelated whitespace change. This makes it more difficult to review.
  • I'm not sure, whether we need this getTypeImage() method. It looks like, we should not add another one of these methods, since we should use proper type resolution (and get the real type via getType()). The getTypeImage() methods look to me like workarounds for not fully implemented type resolution. Can you give it a try, whether we don't need it, if you use TypeHelper?
Show resolved Hide resolved pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTLiteral.java Outdated
Show resolved Hide resolved ...java/src/test/java/net/sourceforge/pmd/lang/java/ast/ASTLiteralTest.java Outdated
Show resolved Hide resolved ...e/pmd/lang/java/rule/bestpractices/xml/JUnitTestsShouldIncludeAssert.xml Outdated
boolean methodIsAssertAll = "assertAll".equals(methodName);

String varName = tokens[0];
boolean variableTypeIsSoftAssertion = variables.containsKey(varName) && variables.get(varName).getTypeImage().endsWith("SoftAssertions");

This comment has been minimized.

@adangel

adangel Nov 5, 2018

Member

We probably should use net.sourceforge.pmd.lang.java.typeresolution.TypeHelper.isA(TypeNode, String) for the type check, e.g. TypeHelper.isA(variables.get(varName), "org.assertj.core.api.SoftAssertions").
Note: we need to add a method in TypeHelper, that deals with a TypeNameDeclaration and a class given as String.

Show resolved Hide resolved ...e/pmd/lang/java/rule/bestpractices/xml/JUnitTestsShouldIncludeAssert.xml
Show resolved Hide resolved ...e/pmd/lang/java/rule/bestpractices/xml/JUnitTestsShouldIncludeAssert.xml

@adangel adangel changed the title [java] [junit] Recognize AssertJ soft assertions as valid assert statements [java] JUnitTestsShouldIncludeAssert: Recognize AssertJ soft assertions as valid assert statements Nov 5, 2018

@ledoyen

This comment has been minimized.

Copy link
Contributor

ledoyen commented Nov 6, 2018

About trailling spaces, I thought as they are usually considered a bad practice it would be an improvement if they are removed.

@ledoyen ledoyen force-pushed the ledoyen:feature/recognize_assertj_soft_asserts_as_assert branch 2 times, most recently from b58471b to 85f3a0a Nov 12, 2018

@ledoyen

This comment has been minimized.

Copy link
Contributor

ledoyen commented Nov 12, 2018

I tried everything suggested and it looks quite good.

However, I am still not sure about ASTLiteral modifications as the image needs to be supplied before a call to one of the set methods, what is called connascence of execution.

Any suggestion would be appreciated 😃

@ledoyen ledoyen force-pushed the ledoyen:feature/recognize_assertj_soft_asserts_as_assert branch from 85f3a0a to cc8181e Nov 12, 2018

@adangel adangel self-assigned this Nov 13, 2018

@adangel

This comment has been minimized.

Copy link
Member

adangel commented Nov 14, 2018

@ledoyen Thanks for the update!

However, I am still not sure about ASTLiteral modifications as the image needs to be supplied before
a call to one of the set methods, what is called connascence of execution.

We should not need to call setType() explicitly inside ASTLiteral - this is supposed to be set by the ClassTypeResolver / TypeResolutionFacade. I'll have a look, whether we have a problem there.
The TypeResolution part is executed before the rule is executed - so the rule can expect that the type is set correctly already.

I'm a bit concerned about the removed violations in the report: http://chunk.io/pmd/135a4be9e1654a969ca77f01bdf3f2fe/diff/index.html

They are mostly false positives, so not reporting them would be correct. But it sound too good, that we suddenly follow the call flow and detect, that an assert is executed indirectly in another method....

E.g. in this method, I actually don't see an assert:
https://github.com/spring-projects/spring-framework/blob/v5.0.6.RELEASE/spring-aop/src/test/java/org/springframework/aop/framework/MethodInvocationTests.java#L61

There is another method in the same test class, which indeed has an assert statement - so maybe we mix the asserts from different test methods?

adangel added some commits Nov 14, 2018

[java] TypeResolution / SymbolFacade for var inferred types
The returned type of the variable name declaration is now also
the inferred type.

Note: getTypeImage() still returns null, since there is no TypeNode
and therefore no image for "var" variable declarations.

@adangel adangel added this to the 6.10.0 milestone Nov 14, 2018

@ledoyen

This comment has been minimized.

Copy link
Contributor

ledoyen commented Nov 14, 2018

Yes I saw that many violations where removed, but not being familiar with the build process of pmd, I just tried to reproduce one of them with another test case for JUnitTestsShouldIncludeAssert (this I did not commited). But violations where raised as expected in this local version of mine. I still do not understand why these violations are not raised during CI analysis.

I will try with your suggestion (2 methods ,one with an assert statement, the other without) and see where it goes.

@adangel

This comment has been minimized.

Copy link
Member

adangel commented Nov 14, 2018

I've now pushed some more commits to your branch:

  • I've reverted the change for ASTLiteral completely, since it was not necessary: The type resolution already worked for all the var cases correctly, even for the indirect one (var bar = 1L; var foo = bar;).
  • One problem for the VariableNameDeclarationTest was this: It used a very old base test, which only setup the symbol facade. This is good, if you just want to find symbols. But as soon as you want to resolve types, we need the type resolution - that's why you have seen the missing type on ASTLiteral.
  • I've mostly undone the changes for VariableNameDeclaration: for the inferred variables, there is no type node and therefore no type image - so returning null for the image is ok. I've modified getType() though, to return the inferred type (if the declarator node has one) - this was the piece, that was missing to get the type from var declarations.
  • I didn't need to change the rule - these changes are good.
  • Regarding the missing violations: I also tried some test cases and the rule still works (I've seen, we have already such a test case). So everything is fine. Also the test case from Springframework works inside PMD - and that's where it becomes interesting: Our pmdtester tool doesn't set the auxclasspath yet (see pmd/pmd-regression-tester#48) - that's why these violations are not found. But I don't understand yet, why these were found before this change...

Let's see what the new report tells us....

@adangel

This comment has been minimized.

Copy link
Member

adangel commented Nov 14, 2018

Ok, found the problem:

WARNING: Exception applying rule JUnitTestsShouldIncludeAssert on file /home/andreas/PMD/source/pmd-regression-tester/target/repositories/checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/api/DetailASTTest.java, continuing with next rule
java.lang.NullPointerException
	at net.sourceforge.pmd.lang.java.typeresolution.TypeHelper.isA(TypeHelper.java:147)
	at net.sourceforge.pmd.lang.java.rule.bestpractices.JUnitTestsShouldIncludeAssertRule.isSoftAssertionStatement(JUnitTestsShouldIncludeAssertRule.java:224)
	at net.sourceforge.pmd.lang.java.rule.bestpractices.JUnitTestsShouldIncludeAssertRule.containsExpectOrAssert(JUnitTestsShouldIncludeAssertRule.java:63)
	at net.sourceforge.pmd.lang.java.rule.bestpractices.JUnitTestsShouldIncludeAssertRule.containsExpectOrAssert(JUnitTestsShouldIncludeAssertRule.java:69)
	at net.sourceforge.pmd.lang.java.rule.bestpractices.JUnitTestsShouldIncludeAssertRule.containsExpectOrAssert(JUnitTestsShouldIncludeAssertRule.java:69)
	at net.sourceforge.pmd.lang.java.rule.bestpractices.JUnitTestsShouldIncludeAssertRule.containsExpectOrAssert(JUnitTestsShouldIncludeAssertRule.java:69)
	at net.sourceforge.pmd.lang.java.rule.bestpractices.JUnitTestsShouldIncludeAssertRule.visit(JUnitTestsShouldIncludeAssertRule.java:48)
	at net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration.jjtAccept(ASTMethodDeclaration.java:33)
	at net.sourceforge.pmd.lang.java.ast.AbstractJavaNode.childrenAccept(AbstractJavaNode.java:60)
	at net.sourceforge.pmd.lang.java.rule.AbstractJavaRule.visit(AbstractJavaRule.java:84)
	at net.sourceforge.pmd.lang.java.rule.AbstractJavaRule.visit(AbstractJavaRule.java:130)
	at net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceBodyDeclaration.jjtAccept(ASTClassOrInterfaceBodyDeclaration.java:43)
...

So, without a proper auxclasspath, the call to loadClass might actually return null.

I wonder, why these additional errors do not end up in the report...

adangel added some commits Nov 15, 2018

Avoid NPE if class is not found
The class org.assertj.core.api.AbstractSoftAssertions might not be
on the auxclasspath, which would cause this NPE.

@adangel adangel merged commit 5f2d9b3 into pmd:master Nov 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ledoyen

This comment has been minimized.

Copy link
Contributor

ledoyen commented Nov 16, 2018

@adangel Thanks for your great work and help !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment