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] JUnitTestsShouldIncludeAssertRule should support `this.exception` as well as just `exception` #647

Closed
blerner opened this Issue Sep 24, 2017 · 6 comments

Comments

Projects
None yet
5 participants
@blerner
Copy link

blerner commented Sep 24, 2017

RuleSet: JUnit

Description: Related to #285, the following variant on the code there will produce an error about missing assertions:

public class DigitalAssetManagerTest {
  @Rule
  public final ExpectedException exception = ExpectedException.none();
  @Test
  public void throwsIllegalArgumentExceptionIfIconIsNull() {
    this.exception.expect(IllegalArgumentException.class);
    this.exception.expectMessage("Icon is null, not a file, or doesn't exist.");
    new DigitalAssetManager(null, null);
  }
}

If either of the this.exceptions are changed to merely exception, then the reported error goes away. While the code samples in https://github.com/junit-team/junit4/wiki/Rules don't display the use of this. as a general matter, it is semantically allowed.

This is run with v5.8.1 of PMD via the command line, so it's current behavior.

@blerner

This comment has been minimized.

Copy link

blerner commented Mar 29, 2018

Bump? This is still current with 6.2.0, and I'd love to be able to fix this bug in time for my next semester's students... If I can help test a fix, I'd be happy to.

@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Mar 29, 2018

@blerner thanks for your concern and sorry for the lack of response. I fear we had our hands full last year just wrapping up 6.0.0, and this year's start hasn't been easier on us.

The issue remains unscheduled, but any PRs are welcomed and will definitely help getting this in place for 6.3.0 (to be released by late April as scheduled). The definitive fix for this would put some pressure on symbol table / type resolution, but I think there is a simpler fix that could cover all except the most corner-cases and should be easy enough for anyone willing to tackle it.

The problem lies here:

String varname = img.split("\\.")[0];
if (!expectables.containsKey(varname)) {

Instead of just checking if the subindex 0 is among expectables, we should check if subindex 0 is this, if so, we check index 1 against expectables, otherwise we keep checking index 0.

Similarly, when checking the method call itself on:

if (occ.getLocation() == name && img.startsWith(varname + ".expect")) {

we should make sure to check with the "this." prefix if it was present.

Of course, a test case would have to be added under JUnitTestsShouldIncludeAssert.xml.

@blerner

This comment has been minimized.

Copy link

blerner commented Sep 27, 2018

This seems to be fixed in 6.7.0, based on a really quick check just now. But it would be good to have a real regression test in PMD to confirm this is a deliberate fix...

orimarko added a commit to orimarko/pmd that referenced this issue Nov 11, 2018

Add Test for issue pmd#647
Add test to verify fix for issue pmd#647:
JUnitTestsShouldIncludeAssertRule should support `this.exception` as well as just `exception`
@orimarko

This comment has been minimized.

Copy link
Contributor

orimarko commented Nov 12, 2018

I added a test and it failed with assertion with this.

import org.junit.*;
public class DigitalAssetManagerTest {
  @Rule
  public final ExpectedException exception = ExpectedException.none();
  @Test
  public void throwsIllegalArgumentExceptionIfIconIsNull() {
    this.exception.expect(IllegalArgumentException.class);
    this.exception.expectMessage("Icon is null, not a file, or doesn't exist.");
    new DigitalAssetManager(null, null);
  }
}
@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Nov 13, 2018

@blerner according to #1457, it's not fixed. The PR is failing due to this test scenario. I guess this indeed needs fixing.

@oowekyala

This comment has been minimized.

Copy link
Member

oowekyala commented Nov 13, 2018

Some more info: the AST radically changes structure when a PrimaryExpression starts with this.
Without this, exception.expect(...) is parsed as

PrimaryExpression
  PrimaryPrefix
    Name[@Image = "exception.expect"]
  PrimarySuffix
    Arguments

With this, this.exception.expect(...) is parsed as

PrimaryExpression
  PrimaryPrefix[@ThisModifier = "true", @Image = null ]
  PrimarySuffix[@Image = "exception"]
  PrimarySuffix[@Image = "expect"]
  PrimarySuffix
    Arguments

The rule only handles the first parse tree for now, because it searches for a Name node:

which in the second parse tree doesn't exist (well it may, but much deeper in the tree, and so that's not what we're looking for).

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