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] Simplify check for 'Test' annotation in JUnitTestsShouldIncludeAssertRule. #1374

Merged
merged 2 commits into from Oct 10, 2018

Conversation

Projects
None yet
4 participants
@winder

winder commented Oct 8, 2018

Please, prefix the PR title with the language it applies to within brackets, such as [java] or [apex]. If not specific to a language, you can use [core]

Before submitting a PR, please check that:

  • The PR is submitted against master. The PMD team will merge back to support branches as needed.
  • ./mvnw test passes.
  • ./mvnw pmd:check passes.
  • ./mvnw checkstyle:check passes. Check this for more info

PR Description:

Resolves #1365

mvnw test failed for me on master so I wasn't able to run it.

@winder winder referenced this pull request Oct 8, 2018

Merged

[java] Add missing null check AbstractJavaAnnotatableNode #1375

3 of 4 tasks complete
@pmd-test

This comment has been minimized.

pmd-test commented Oct 8, 2018

1 Message
📖 This changeset introduces 0 new violations and 0 new errors,
removes 0 violations and 2 errors. Full report
This changeset introduces 0 new violations and 0 new errors,
removes 0 violations and 2 errors. Full report

Generated by 🚫 Danger

@adangel

This comment has been minimized.

Member

adangel commented Oct 8, 2018

@winder, Thanks for the PRs. It looks good.

Did you verify #1365 ? Does your change fix the issue?

mvnw test and mvnw pmd:check fail for me on master so I wasn't able to run them.

This should not be the case - what error(s) do you get?

@adangel adangel added this to the 6.9.0 milestone Oct 8, 2018

@winder

This comment has been minimized.

winder commented Oct 8, 2018

Testing

yes, the reported issue. I split the original test into 2 files and ran them against master and my branch using these files:

Style1.java

import org.junit.Test;

class Style {
  // This triggers the JUnitTestsShouldIncludeWarning rule
  @org.junit.Test(expected = IllegalArgumentException.class)
  public void moveOutOfBoundsFrom() {
    doSomething();
  }
}

Style2.java

import org.junit.Test;

class Style {
  // This does not
  @Test(expected = IllegalArgumentException.class)
  public void moveOutOfBoundsFrom() {
    doSomething();
  }
}

Reproduced on master:

/opt/pmd/pmd-dist/target/pmd-bin-6.9.0-SNAPSHOT/bin$ ./run.sh pmd -d Style1.java -f text -R category/java/bestpractices.xml
Oct 08, 2018 12:11:15 PM net.sourceforge.pmd.PMD processFiles
WARNING: This analysis could be faster, please consider using Incremental Analysis: https://pmd.github.io/latest/pmd_userdocs_incremental_analysis.html
/opt/pmd/pmd-dist/target/pmd-bin-6.9.0-SNAPSHOT/bin/Style1.java:1:	Avoid unused imports such as 'org.junit.Test'
/opt/pmd/pmd-dist/target/pmd-bin-6.9.0-SNAPSHOT/bin/Style1.java:6:	JUnit tests should include assert() or fail()

/opt/pmd/pmd-dist/target/pmd-bin-6.9.0-SNAPSHOT/bin$ ./run.sh pmd -d Style2.java -f text -R category/java/bestpractices.xml
Oct 08, 2018 12:11:27 PM net.sourceforge.pmd.PMD processFiles
WARNING: This analysis could be faster, please consider using Incremental Analysis: https://pmd.github.io/latest/pmd_userdocs_incremental_analysis.html

Resolved on this branch:

/opt/pmd/pmd-dist/target/pmd-bin-6.9.0-SNAPSHOT/bin$ ./run.sh pmd -d Style1.java -f text -R category/java/bestpractices.xml
Oct 08, 2018 12:24:15 PM net.sourceforge.pmd.PMD processFiles
WARNING: This analysis could be faster, please consider using Incremental Analysis: https://pmd.github.io/latest/pmd_userdocs_incremental_analysis.html
/opt/pmd/pmd-dist/target/pmd-bin-6.9.0-SNAPSHOT/bin/Style1.java:1:	Avoid unused imports such as 'org.junit.Test'

/opt/pmd/pmd-dist/target/pmd-bin-6.9.0-SNAPSHOT/bin$ ./run.sh pmd -d Style2.java -f text -R category/java/bestpractices.xml
Oct 08, 2018 12:24:26 PM net.sourceforge.pmd.PMD processFiles
WARNING: This analysis could be faster, please consider using Incremental Analysis: https://pmd.github.io/latest/pmd_userdocs_incremental_analysis.html

mvnw test / mvnw pmd:check issues

I tried running pmd:check again, previously there were about 40 errors, but now it seems to be working. I'm not sure what changed.

mvnw test still fails due to missing classes, should I create a new issue for this?

mvnw test errors during PMD Test Framework:

[ERROR] Tests run: 2, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 0.353 s <<< FAILURE! - in net.sourceforge.pmd.testframework.RuleTstTest
[ERROR] shouldCallStartAndEnd(net.sourceforge.pmd.testframework.RuleTstTest)  Time elapsed: 0.046 s  <<< ERROR!
java.lang.NoClassDefFoundError: org/jaxen/Function
	at net.sourceforge.pmd.testframework.RuleTstTest.shouldCallStartAndEnd(RuleTstTest.java:47)
Caused by: java.lang.ClassNotFoundException: org.jaxen.Function
	at net.sourceforge.pmd.testframework.RuleTstTest.shouldCallStartAndEnd(RuleTstTest.java:47)

[ERROR] shouldAssertLinenumbersSorted(net.sourceforge.pmd.testframework.RuleTstTest)  Time elapsed: 0.009 s  <<< ERROR!
java.lang.NoClassDefFoundError: Could not initialize class net.sourceforge.pmd.lang.xpath.Initializer
	at net.sourceforge.pmd.testframework.RuleTstTest.shouldAssertLinenumbersSorted(RuleTstTest.java:92)
@adangel

This comment has been minimized.

Member

adangel commented Oct 8, 2018

Awesome! Then we should just add a test case for the case, you just fixed.
Please extend the file https://github.com/pmd/pmd/blob/master/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/JUnitTestsShouldIncludeAssert.xml with your two test cases, thanks!

About the build problem: What's the result, if you execute ./mvnw clean verify? That's executes all the steps mentioned above (it builds, runs the tests, runs pmd, and runs checkstyle).

The problem with just running the "test" goal is, that we don't have the jaxen library yet... (it's only available at the end of the "package" goal, we need to shade the interface org.w3c.dom.UserDataHandler away, in order to avoid duplicated classes - this interface is already provided as a java api...).
I'll update the pull-request template, to use just ./mvnw clean verify.

@winder

This comment has been minimized.

winder commented Oct 8, 2018

Yes, ./mvnw clean verify runs fine

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Oct 8, 2018

@jsotuyod jsotuyod added the is:WIP label Oct 8, 2018

Will Winder

@winder winder force-pushed the winder:issue-1365 branch from a29a8c0 to 5b32457 Oct 8, 2018

@winder

This comment has been minimized.

winder commented Oct 9, 2018

@jsotuyod @adangel this should be good to go.

@jsotuyod jsotuyod merged commit 5b32457 into pmd:master Oct 10, 2018

1 check passed

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

jsotuyod added a commit that referenced this pull request Oct 10, 2018

@jsotuyod jsotuyod removed the is:WIP label Oct 10, 2018

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Oct 10, 2018

@winder merged, thanks again for the PR! You are halfway there to getting your hacktoberfest t-shirt if you are taking part of that!

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