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] Adding test for Issue #647 #1457

Merged
merged 4 commits into from Dec 4, 2018

Conversation

Projects
None yet
4 participants
@orimarko
Copy link
Contributor

orimarko commented Nov 12, 2018

Before submitting a PR, please check that:

  • [ X ] The PR is submitted against master. The PMD team will merge back to support branches as needed.
  • [ X ] ./mvnw clean verify passes. This will build and test PMD, execute PMD and checkstyle rules. Check this for more info

PR Description:
Adding test for Issue #647 JUnitTestsShouldIncludeAssertRule should support this.exception as well as just exception

orimarko added some commits Nov 11, 2018

Add Test for issue #647
Add test to verify fix for issue #647:
JUnitTestsShouldIncludeAssertRule should support `this.exception` as well as just `exception`
Fix Test
Fix Test add `import org.junit.*;`
@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Nov 12, 2018

@orimarko thanks for the PR!

#647 is about using this.exception and not just exception (not using this is already covered in a couple of test cases

<test-code>
<description>#285 Issues with @Rule annotation and ExpectedException</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>7</expected-linenumbers>
<code><![CDATA[
import org.junit.*;
public class SimpleExpectedExceptionTest {
@Rule
public ExpectedException thrown = ExpectedException.none();
@Test
public void throwsExceptionWithSpecificType() {
throw new NullPointerException(); // No expect! this is a violation
}
@Test
public void throwsIllegalArgumentExceptionIfIconIsNull() {
thrown.expect(IllegalArgumentException.class);
thrown.expectMessage("Icon is null, not a file, or doesn't exist.");
new DigitalAssetManager(null, null);
}
}
]]></code>
</test-code>
<test-code>
<description>#285 Follow-up for @org.junit.Rule</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import org.junit.*;
public class SimpleExpectedExceptionTest {
@org.junit.Rule
public ExpectedException thrown = ExpectedException.none();
@Test
public void throwsExceptionWithSpecificType() {
thrown.expect(NullPointerException.class);
throw new NullPointerException();
}
}
]]></code>
</test-code>
)

And for the issue to be resolved, it should expect 0 warnings, not 1.

@jsotuyod jsotuyod changed the title Adding test for Issue #647 [java] [java] Adding test for Issue #647 Nov 12, 2018

@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Nov 13, 2018

@orimarko thanks! that test case is perfect. Unfortunately, the build fails because the issue isn't actually resolved as the original reported believed.

Nontheless, I'd already posted some insight into how to actually fix this: #647 (comment)

If you feel up to it, you can probably get it done easily and wrap up a very meaningful first contribution to PMD!

@adangel

This comment has been minimized.

Copy link
Member

adangel commented Nov 25, 2018

Hi @orimarko ,

we are planning to release PMD 6.10.0 in about 2 weeks. Do you have time to work on this issue? It would be awesome, if we could include a fix in the release.

@adangel adangel added the is:WIP label Nov 28, 2018

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

@oowekyala oowekyala merged commit c68bfaf into pmd:master Dec 4, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

adangel added a commit that referenced this pull request Dec 4, 2018

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