Custom rule to verify that "assertThat()" is followed by a predicate call?

David_Karr · April 5, 2021, 12:00pm

I was starting to read about how to write custom Java rules, when I noticed that the library I want to write an assertion for is already being used, at least in the sample library. The library is AssertJ. In the projects I’m involved with, I’ve seen many examples of broken assertions like the following:

assertThat(....getSomeValue().equals(30));

This was obviously written by a developer who didn’t understand “make tests fail before they pass”.

This should be something like this:

assertThat(...getSomeValue()).isEqualTo(30);

The custom rule I want to write would report calls to method “assertThat()” that don’t call predicates on the result. In fact, I wrote a custom xpath rule to do exactly this years ago, but I didn’t fully implement it at the time.

I am proceeding to examine the rules api so I can write this rule myself, but I was wondering, if elements of the SonarQube universe are already using this library, perhaps someone has already written a custom plugin to check for this? I performed a rudimentary search for something like this, but I didn’t find it.

I have proceeded to try to write this plugin, but I’m running into issues.

Modeling it like the example, my test file is the following:

import static org.assertj.core.api.Assertions.*;

public class AssertThatWithNoPredicate {

	public Foo foo  = new Foo();

	@Test
	public void assertSomething() throws Exception {
		assertThat(foo.getStuff().equals(34));
	}

	public static class Foo {
		private int stuff;

		public int getStuff() { return stuff; }
		public void setStuff(int aStuff) { this.stuff = stuff; }
	}
}

When I got this working with XPath a few years ago, I ended up with the following expression (it’s still in the SonarQube archived forum ):

//classDeclaration[IDENTIFIER[ends-with(@tokenValue, 'Test')]]//expression/primary[@tokenValue='assertThat']

So, that tells me I need to implement “visitExpressionStatement”, and I tried printing out the firstToken text value, and that showed that I did find that expression. However, I don’t know where to go from here. I’m not sure what to do with “primary”, or whether there’s a different wording for this in the Java api.

David_Karr · April 3, 2021, 8:38pm

I reexamined the old thread where I asked about this before: SonarQube Users (archive) - Write a custom XPath task that looks for a method that is NOT followed by a chained method call? .

I realized that the reply gave me details of what the java solution should do, but I didn’t really understand the logic for what he was suggesting, and when I implemented it as close to what I think he was saying, it didn’t appear to work.

From the archived thread, he said:

You can expect every MethodInvocationTree whose name is "assertThat"
(use "methodSelect()" to verify that) to be within the "expression()"
part of a MemberSelectExpressionTree.

In practice, you'll realize that you don't have access to parent nodes
in the AST. So instead, you override both the visit member select and
method invocation methods. In member select: if in the method select,
your "expression()" is a call to "assertThat(…)", then do not visit
that "expression()" sub-tree. In method invocation: If
"methodSelect()" is a call to "assertThat(…)", then raise an issue.

I tried a couple of different variations of this, but I couldn’t get this to work.

David_Karr · April 4, 2021, 12:00am

Looking at this again, I was wondering about the “parent” property, along with the statement he made that I wouldn’t have access to the parent nodes. I don’t quite understand that. I tried printing and checking the parent in “visitMethodInvocation”, and I had no trouble getting it.

I finally just printed the methodSelect’s firstToken, kind, and parent kind, and noted what those values were when I got into this method with a valid “assertThat()”, as opposed to an invalid one. I tried simply changing the condition to match those situations. I still don’t understand the logic, but this results in it calling “reportIssue()” when there is an issue, and not when there is not.

However, the case where it calls “reportIssue()” still fails the test, for some weird “Unexpected at …” error.

I’ll now include my code. I"m omitting the package and imports for all of these.

Here is the meat of the visitor class:

public class AssertThatLackingPredicateCheck extends BaseTreeVisitor implements JavaFileScanner {
	private JavaFileScannerContext context;

	@Override
	public void scanFile(JavaFileScannerContext context) {
		this.context = context;
		scan(context.getTree());
	}

	@Override
	public void visitMemberSelectExpression(MemberSelectExpressionTree tree) {
		System.out.println("In visitMemberSelectExpression.");
		scan(tree.annotations());
		System.out.println("tree.expression.firstToken[" + tree.expression().firstToken().text() + "]");
		scan(tree.expression());
		scan(tree.identifier());
		System.out.println("Exiting visitMemberSelectExpression.");
	}

	@Override
	public void visitMethodInvocation(MethodInvocationTree tree) {
		System.out.println("In visitMethodInvocation.");
		System.out.println("methodSelect[" + tree.methodSelect().firstToken().text() +
						   "] kind[" + tree.methodSelect().kind() +
						   "] parent.kind[" + tree.parent().kind() +
						   "]");
		scan(tree.methodSelect());
		scan(tree.typeArguments());
		scan(tree.arguments());
		if (tree.methodSelect().firstToken().text().equals("assertThat") &&
			tree.methodSelect().kind() == Kind.IDENTIFIER &&
			tree.parent().kind() == Kind.EXPRESSION_STATEMENT) {
			System.out.println("Reporting issue.");
			context.reportIssue(this, tree,
								"Calls to assertThat have to chain predicate method calls, or the assertion does nothing.");
		}
		System.out.println("Exiting visitMethodInvocation.");
	}
}

Here is my test class:

public class AssertThatLackingPredicateCheckTest {
	@Test
	public void assertThatWithNoPredicate() throws Exception {
		JavaCheckVerifier.newVerifier()
			.onFile("src/test/files/AssertThatWithNoPredicate.java")
			.withCheck(new AssertThatLackingPredicateCheck())
			.verifyIssues();
	}

	@Test
	public void assertThatWithValidPredicate() throws Exception {
		JavaCheckVerifier.newVerifier()
			.onFile("src/test/files/AssertThatWithValidPredicate.java")
			.withCheck(new AssertThatLackingPredicateCheck())
			.verifyNoIssues();
	}
}

And here are my two test files:

This one should report an issue:

public class AssertThatWithNoPredicate {
	public Foo foo  = new Foo();

	@Test
	public void assertSomething() throws Exception {
		assertThat(foo.getStuff().equals("abc"));
	}

	public static class Foo {
		private String stuff;

		public String getStuff() { return stuff; }
		public void setStuff(String aStuff) { this.stuff = stuff; }
	}
}

This one should not report an issue:

public class AssertThatWithValidPredicate {
	public Foo foo  = new Foo();

	@Test
	public void assertSomething() throws Exception {
		assertThat(foo.getStuff()).isEqualTo("abc");
	}

	public static class Foo {
		private String stuff;

		public String getStuff() { return stuff; }
		public void setStuff(String aStuff) { this.stuff = stuff; }
	}
}

The “valid” test passes, but the “no predicate” test fails with the following (abbreviated):

java.lang.AssertionError: Unexpected at [10]
	at org.sonar.java.testing.InternalCheckVerifier.assertMultipleIssues(InternalCheckVerifier.java:308)
	at org.sonar.java.testing.InternalCheckVerifier.checkIssues(InternalCheckVerifier.java:232)
	at org.sonar.java.testing.InternalCheckVerifier.verifyAll(InternalCheckVerifier.java:223)
	at org.sonar.java.testing.InternalCheckVerifier.verifyIssues(InternalCheckVerifier.java:168)
	at org.sonar.samples.java.checks.AssertThatLackingPredicateCheckTest.assertThatWithNoPredicate(AssertThatLackingPredicateCheckTest.java:12)

David_Karr · April 4, 2021, 5:12pm

On a related note, I see that sonarsource actually has six assertj-specific rules (Rules explorer ), but this issue is unfortunately not addressed.

David_Karr · April 8, 2021, 3:04pm

And I have now discovered that this effort was unnecessary, as there is already an existing rule that covers this: Rules explorer . I didn’t see it before because it doesn’t have the “assertj” tag. It would be good to change that, although it would obviously also need the other related framework tags.

Quentin · April 8, 2021, 3:04pm

Hello @David_Karr

Glad to see that you managed to find an answer to your question by yourself. Indeed, we have 45 rules targetting test code, and the situation you described is already supported by the one you mention. I updated the tags of the rule to include “assertj”, it will hopefully avoid future confusion.

Best,
Quentin