Java analysis yields false positives for junit 4 vintage tests

Versions:

  • SonarQube community edition 8.5.1
  • Maven analyzer: org.sonarsource.scanner.maven:sonar-maven-plugin:3.7.0.1746

Steps to reproduce:

  • analyze the simplest Java project that uses Java 8, has a dependency on junit 5 but contains some legacy junit 4 tests
  • expected outcome: public methods in junit4 tests are not identified as issues
  • actual outcome: public methods in junit4 are flagged as issues

The relevant dependency, in a maven project including junit 5:

  <groupId>org.junit.vintage</groupId>
  <artifactId>junit-vintage-engine</artifactId>
  <version>5.5.2</version>

A relevant code sample:

import org.junit.Test;
import static org.junit.Assert.assertTrue;

public class SomeTest {
	@Test
	public void testNothing() {
		assertTrue(true);
	}
}

It is unreasonable to expect large codebases to upgrade all of their tests to junit 5, even if they start writing new junit 5 tests for new/changed code, or to manually flag all issues with public test methods found by the analyzer as false positives manually.

Hello @AnonymousCoward

Before starting, you are talking about S5793, right?

It is unreasonable to expect large codebases to upgrade all of their tests to junit 5

I agree, and in fact, this is what the rule is about:

maintaining both systems [JUnit 4 and 5] is a temporary solution. This rule flags all the annotations from JUnit4 which would need to be migrated to JUnit5, hence helping migration of a project.

By reporting “info” code smells (lowest severity possible), this rule is here to help you to track what remains to be done, and most importantly prevent any new code to use outdated annotations in new code.
It goes with the Clean as You Code idea, we don’t expect all issues to be fixed right away, the code works just fine as it is and, as you just said, it is simply unreasonable. If you focus on cleaning as you code though, all these issues will eventually disappear.

By the way, this rule is not in the default profile, hence not enable by default, exactly because it should be explicitly acknowledged that the goal is to eventually get rid of JUnit 4.

Does it clarify the situation?

I understand what you’re saying, but I experience something different. I installed sonarqube community with default settings using this helm chart, which, to my understanding, is either the official chart, or the closest there is to an official chart. After such an installation, the rule is enabled by default.

If a larger codebase decides to update to a newer framework version, which pulls in Junit 5, although all of its tests are Junit 4, and this rule is enabled by default, they will most likely manually close all public/package private issues as won’t fix. Moreover, they might decide to stay with Junit 4 forever - mixing two test styles in the same codebase doesn’t improve readability at all, upgrading all tests without functional changes is expensive and doesn’t add value. Such sonarqube users definitely wouldn’t want this rule to be enabled by default - and I don’t expect them to be few.

I have solved my problem using some batch modification feature in the UI. So as long as you don’t intend to do anything about this, I don’t mind if things stay as they are. With the sonarqube community edition as installed by the helm chart mentioned above, however, the rule seems enabled by default. Maybe it’s worth a check.

Either way, a much nicer variant of this rule, specifically for Junit 4 tests, in my opinion, would be to put an “Upgrade your test to Junit 5” on Junit 4 tests. Complaining about public in Junit 4 tests doesn’t make much sense.

the rule is enabled by default.

Something is still not clear to me: what rule are we talking about?

  • S5786: JUnit5 test classes and methods should have default package visibility
    or
  • S5793: Migrate your tests from JUnit4 to the new JUnit5 annotations

Or another one?

I am so stupid! The tests in that codebase are all supposed to be Junit4, but for whatever reason some Junit5 annotations have sneaked into some tests. Sorry for wasting your time.

No worries, glad to see that we managed to somehow find an answer to your question.

Best,
Quentin