You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@jmeter.apache.org by ham1 <gi...@git.apache.org> on 2017/11/29 22:59:56 UTC

[GitHub] jmeter pull request #345: Checkstyle

GitHub user ham1 opened a pull request:

    https://github.com/apache/jmeter/pull/345

    Checkstyle

    ## Description
    * Updated to latest checkstyle (v8.5)
    * Added many more rules to checkstyle
    * Included checking of test files and more file types
    
    ## Motivation and Context
    Automatically enforcing some of the coding standards/conventions helps improve readability and prevents commits like "Tab police" or commits which only change formatting thus cluttering the history (or leaving it in an inconsistent state to micro-annoy subsequent readers).
    
    ## How Has This Been Tested?
    `ant clean checkstyle -Djava.awt.headless=true -Dskip.bug52310=true -Dskip.test_https=true test`
    
    ## Screenshots (if appropriate):
    
    ## Types of changes
    <!--- What types of changes does your code introduce? Delete as appropriate -->
    - Dev enhancement
    
    ## Checklist:
    <!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
    <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
    - [x] My code follows the [code style][style-guide] of this project.
    - [x] I have updated the documentation accordingly.
    
    [style-guide]: https://wiki.apache.org/jmeter/CodeStyleGuidelines


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/ham1/jmeter checkstyle

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/jmeter/pull/345.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #345
    
----
commit 2211ef54083c63408c5d26a6b3efc094217a212d
Author: Graham Russell <gr...@ham1.co.uk>
Date:   2017-11-20T23:15:30Z

    Added JavaDoc check for methods

commit c522c65a56e7235d9ff08863a06f78fd46ed677c
Author: Graham Russell <gr...@ham1.co.uk>
Date:   2017-11-23T22:25:18Z

    Added checkstyle rules: AvoidStarImport, Redundant and Unused Import

commit 2c42a045f23086f28530ebc5131824c6749a3094
Author: Graham Russell <gr...@ham1.co.uk>
Date:   2017-11-26T10:18:10Z

    Updated to latest checkstyle, 8.4. Included test and properties files.
    Added more rules, e.g. max file length, max 2 blank lines, no empty catch blocks.

commit f22813aeaa1425a145481d6d997443b783391246
Author: Graham Russell <gr...@ham1.co.uk>
Date:   2017-11-26T19:59:09Z

    Added IllegalThrows and fixed issues

commit 7833686519d94f3746918aa16d2f7bbd51002c37
Author: Graham Russell <gr...@ham1.co.uk>
Date:   2017-11-27T21:45:45Z

    WIP: MissingSwitchDefault and ModifiedControlVariable

commit 1b1cc5bde4118dda7e11d076fdef832e02fa3996
Author: Graham Russell <gr...@ham1.co.uk>
Date:   2017-11-28T01:52:42Z

    Added and fixed MultipleVariableDeclarations, NestedForDepth, NestedIfDepth and NestedTryDepth

commit 02d9cb7a9426c8cf6ba099caf1b7d2fbdb57f15b
Author: Graham Russell <gr...@ham1.co.uk>
Date:   2017-11-28T02:12:43Z

    Removed commented out surpression

commit 8a50df25c32158abbd854c4c49765b440129c7a3
Author: Graham Russell <gr...@ham1.co.uk>
Date:   2017-11-28T02:18:35Z

    Updated to checkstyle 8.5

commit c531cd0cf2cee82ba388f76e0b18bd402acc24d2
Author: Graham Russell <gr...@ham1.co.uk>
Date:   2017-11-29T00:44:07Z

    Added more checkstyle rules, and fixes:
    OneStatementPerLine
    SimplifyBooleanExpression
    SimplifyBooleanReturn
    StringLiteralEquality
    SuperFinalize
    UnnecessaryParentheses

commit 7b95f6a8e77b8dcd6e76694b93529bd4ad0edb1b
Author: Graham Russell <gr...@ham1.co.uk>
Date:   2017-11-29T10:15:33Z

    Formatted darcula_theme.xml with a 2 space indent and added NL at EOF

commit 44fc9d3b9431fdb106b2d07016db9e606f87f664
Author: Graham Russell <gr...@ham1.co.uk>
Date:   2017-11-29T15:19:18Z

    Fix mistake caught by unit tests, hurrah for unit tests.

----


---

[GitHub] jmeter issue #345: Checkstyle

Posted by romani <gi...@git.apache.org>.

Github user romani commented on the issue:

    https://github.com/apache/jmeter/pull/345
  
    if master branch of this project (jmeter) start to use about 80 Checks in its config with ERROR severity and no violations reported, we can start to use jmeter in out CI for regression testing. 
    
    just keep enforcing more and more rules, and pin me then you are done.


---

[GitHub] jmeter issue #345: Checkstyle

Posted by pmouawad <gi...@git.apache.org>.

Github user pmouawad commented on the issue:

    https://github.com/apache/jmeter/pull/345
  
    Hi team, anybody wants to merge this one ? if not I’ll do it this week-end. Thanks


---

[GitHub] jmeter issue #345: Checkstyle

Posted by codecov-io <gi...@git.apache.org>.

Github user codecov-io commented on the issue:

    https://github.com/apache/jmeter/pull/345
  
    # [Codecov](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=h1) Report
    > Merging [#345](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=desc) into [trunk](https://codecov.io/gh/apache/jmeter/commit/4d8facc557fda5986837c3da3b2364350f08b368?src=pr&el=desc) will **decrease** coverage by `0.01%`.
    > The diff coverage is `66.66%`.
    
    [![Impacted file tree graph](https://codecov.io/gh/apache/jmeter/pull/345/graphs/tree.svg?token=6Q7CI1wFSh&width=650&src=pr&height=150)](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=tree)
    
    ```diff
    @@             Coverage Diff             @@
    ##             trunk     #345      +/-   ##
    ===========================================
    - Coverage     58.1%   58.08%   -0.02%     
    - Complexity   10111    10121      +10     
    ===========================================
      Files         1154     1154              
      Lines        73961    73952       -9     
      Branches      7342     7337       -5     
    ===========================================
    - Hits         42972    42955      -17     
    - Misses       28509    28515       +6     
    - Partials      2480     2482       +2
    ```
    
    
    | [Impacted Files](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
    |---|---|---|---|
    | [...e/org/apache/jmeter/report/core/SampleBuilder.java](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=tree#diff-c3JjL2NvcmUvb3JnL2FwYWNoZS9qbWV0ZXIvcmVwb3J0L2NvcmUvU2FtcGxlQnVpbGRlci5qYXZh) | `77.77% <ø> (ø)` | `6 <0> (ø)` | :arrow_down: |
    | [...ache/jmeter/control/TestTransactionController.java](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=tree#diff-dGVzdC9zcmMvb3JnL2FwYWNoZS9qbWV0ZXIvY29udHJvbC9UZXN0VHJhbnNhY3Rpb25Db250cm9sbGVyLmphdmE=) | `90% <ø> (ø)` | `2 <0> (ø)` | :arrow_down: |
    | [.../apache/jmeter/testelement/TestNumberProperty.java](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=tree#diff-dGVzdC9zcmMvb3JnL2FwYWNoZS9qbWV0ZXIvdGVzdGVsZW1lbnQvVGVzdE51bWJlclByb3BlcnR5LmphdmE=) | `76.19% <ø> (ø)` | `6 <0> (ø)` | :arrow_down: |
    | [...apache/jmeter/reporters/ResultCollectorHelper.java](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=tree#diff-c3JjL2NvcmUvb3JnL2FwYWNoZS9qbWV0ZXIvcmVwb3J0ZXJzL1Jlc3VsdENvbGxlY3RvckhlbHBlci5qYXZh) | `0% <ø> (ø)` | `0 <0> (ø)` | :arrow_down: |
    | [...jmeter/protocol/jms/control/gui/JMSSamplerGui.java](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=tree#diff-c3JjL3Byb3RvY29sL2ptcy9vcmcvYXBhY2hlL2ptZXRlci9wcm90b2NvbC9qbXMvY29udHJvbC9ndWkvSk1TU2FtcGxlckd1aS5qYXZh) | `84.37% <ø> (ø)` | `9 <0> (ø)` | :arrow_down: |
    | [.../org/apache/jmeter/functions/TestFileToString.java](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=tree#diff-dGVzdC9zcmMvb3JnL2FwYWNoZS9qbWV0ZXIvZnVuY3Rpb25zL1Rlc3RGaWxlVG9TdHJpbmcuamF2YQ==) | `90% <ø> (ø)` | `7 <0> (ø)` | :arrow_down: |
    | [.../apache/jmeter/extractor/gui/HtmlExtractorGui.java](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=tree#diff-c3JjL2NvbXBvbmVudHMvb3JnL2FwYWNoZS9qbWV0ZXIvZXh0cmFjdG9yL2d1aS9IdG1sRXh0cmFjdG9yR3VpLmphdmE=) | `85.59% <ø> (ø)` | `12 <0> (ø)` | :arrow_down: |
    | [...rc/core/org/apache/jmeter/util/HostNameSetter.java](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=tree#diff-c3JjL2NvcmUvb3JnL2FwYWNoZS9qbWV0ZXIvdXRpbC9Ib3N0TmFtZVNldHRlci5qYXZh) | `75% <ø> (ø)` | `11 <0> (ø)` | :arrow_down: |
    | [...apache/jmeter/report/processor/TaggerConsumer.java](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=tree#diff-c3JjL2NvcmUvb3JnL2FwYWNoZS9qbWV0ZXIvcmVwb3J0L3Byb2Nlc3Nvci9UYWdnZXJDb25zdW1lci5qYXZh) | `0% <ø> (ø)` | `0 <0> (ø)` | :arrow_down: |
    | [...nents/org/apache/jmeter/reporters/MailerModel.java](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=tree#diff-c3JjL2NvbXBvbmVudHMvb3JnL2FwYWNoZS9qbWV0ZXIvcmVwb3J0ZXJzL01haWxlck1vZGVsLmphdmE=) | `35% <ø> (ø)` | `28 <0> (ø)` | :arrow_down: |
    | ... and [59 more](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=tree-more) | |
    
    ------
    
    [Continue to review full report at Codecov](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=continue).
    > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
    > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
    > Powered by [Codecov](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=footer). Last update [4d8facc...44fc9d3](https://codecov.io/gh/apache/jmeter/pull/345?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).



---

[GitHub] jmeter issue #345: Checkstyle

Posted by ham1 <gi...@git.apache.org>.

Github user ham1 commented on the issue:

    https://github.com/apache/jmeter/pull/345
  
    @romani it's commented out to stop this PR failing but where you see it commented out was where I had:
    ```
    <module name="ReturnCount">
      <property name="max" value="10"/>
    </module>
    ```


---

[GitHub] jmeter issue #345: Checkstyle

Posted by romani <gi...@git.apache.org>.

Github user romani commented on the issue:

    https://github.com/apache/jmeter/pull/345
  
    Looks like a reason was in `maxForVoid` with default value., Right ?


---

[GitHub] jmeter issue #345: Checkstyle

Posted by ham1 <gi...@git.apache.org>.

Github user ham1 commented on the issue:

    https://github.com/apache/jmeter/pull/345
  
    Yes, that's correct, I some how missed it in the docs.
    
    I've suggested on https://github.com/checkstyle/checkstyle/issues/5305 that the error message is changed to something like:
    `"Return count is 2 (maxForVoid allowed is 1). [ReturnCount10]"`
    or
    `"Void return count is 2 (max allowed is 1). [ReturnCount10]"`
    to perhaps help prevent the confusion.


---

[GitHub] jmeter issue #345: Checkstyle

Posted by romani <gi...@git.apache.org>.

Github user romani commented on the issue:

    https://github.com/apache/jmeter/pull/345
  
    @ham1 , ReturnCount check is commented out.  Please point me to place where you configure it.


---

[GitHub] jmeter issue #345: Checkstyle

Posted by ham1 <gi...@git.apache.org>.

Github user ham1 commented on the issue:

    https://github.com/apache/jmeter/pull/345
  
    @romani, sorry I'm not clear on what, if anything, you'd want me to do/change?


---