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.

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 false positive with inherited @Rule field #1422

Closed
blerner opened this issue Oct 30, 2018 · 3 comments · Fixed by #2899
Closed

[java] JUnitTestsShouldIncludeAssert false positive with inherited @Rule field #1422

blerner opened this issue Oct 30, 2018 · 3 comments · Fixed by #2899
Labels
a:false-positive PMD flags a piece of code that is not problematic
Milestone

Comments

@blerner
Copy link

blerner commented Oct 30, 2018

Affects PMD Version: 6.9.0

Rule: JUnitTestsShouldIncludeAssertRule

Description: Thanks to bug #631, this rule properly looks for @Rule ExceptedException fields, and correctly does not complain about tests that use them. However, it only checks for such rules in the current class. One scenario that's come up in my students' code is writing an abstract test class with a bunch of concrete tests and an abstract factory method, and then several concrete test classes that instantiate the factory with one of several implementations. Additionally, they write some concrete tests in the concrete classes. Now, the @Rule isn't a declared field in the same class as the @Tests that use it, so this rule incorrectly fires again.

I think the solution is extending here to walk up the inheritance tree, so far as it's known (and here, it should be known), and check for inherited @Rule fields...

Code Sample demonstrating the issue:

import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;

abstract class Shape {
  double size;

  Shape(double size) { 
    if (size < 0) {
      throw new IllegalArgumentException("negative size");
    }
    this.size = size; 
  }

  public abstract double area();
}

class Square extends Shape {
  Square(double size) {
    super(size);
  }
  
  public double area() {
    return size * size;
  }
}

class Weird extends Shape {
  Weird(double size) {
    super(size);
  }
  
  public double area() {
    throw new RuntimeException("just 'cause");
  }
}

public abstract class ShapeTester {
  protected abstract Shape factory(double size);

  @Rule
  public ExpectedException e = ExpectedException.none();

  @Test 
  public void notAProblem() {
    e.expect(IllegalArgumentException.class);
    factory(-5);
  }

  public static final class SquareTester extends ShapeTester {
    protected Shape factory(double size) {
      return new Square(size);
    }
  }

  public static final class WeirdTester extends ShapeTester {
    protected Shape factory(double size) {
      return new Weird(size);
    }

    @Test
    public void thisOneTriggersRule() {
      e.expect(RuntimeException.class);
      e.expectMessage("just 'cause");
      factory(42).area();
    }
  }
}

Running PMD through: programmatically

@jsotuyod jsotuyod added the a:false-positive PMD flags a piece of code that is not problematic label Nov 1, 2018
@oowekyala oowekyala changed the title [java] JUnitTestsShouldIncludeAssert false positive [java] JUnitTestsShouldIncludeAssert false positive with inherited @Rule field Nov 5, 2020
oowekyala added a commit to oowekyala/pmd that referenced this issue Nov 5, 2020
@oowekyala oowekyala linked a pull request Nov 10, 2020 that will close this issue
5 tasks
@oowekyala oowekyala added this to the 7.0.0 milestone Nov 10, 2020
@adangel
Copy link
Member

adangel commented Dec 11, 2020

Fixed via #2899 for PMD 7.

@Sandm-HY
Copy link

Hello, so is this issue resolved now?

@jsotuyod
Copy link
Member

Yes, PMD 7 RCs include this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:false-positive PMD flags a piece of code that is not problematic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants