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

Some way to gradually clamp down would be nice #92

Open
hakanai opened this issue Dec 17, 2015 · 4 comments
Open

Some way to gradually clamp down would be nice #92

hakanai opened this issue Dec 17, 2015 · 4 comments
Assignees

Comments

@hakanai
Copy link

hakanai commented Dec 17, 2015

I have been doing a slow push to getting rid of ResourceBundle.getBundle(String)... initially we had about 1,000 calls to it. All the ones in GUI code I decided I would just refactor out to a utility method and then fix the utility method in one place, which works well and allows us to keep all modules on the same config... for now. That wiped out maybe 9/10 of them overall and now it's a slow grind to find acceptable solutions for the rest.

The task is still in progress though and one of the problems I have is that people keep committing new violations, sometimes at a rate faster than I can fix them.

If I uncomment the line in the forbidden signatures, the whole build fails and people complain because they can't get a build, but not loudly enough to make the people who wrote the offending code fix it.

If I comment the line out, the build works but now new additions are permitted and increase. In some situations, people have even reverted the fixes I just made in the previous day!

I realise that I could suppress the warning in the 100 or so places where it exists, but what I have seen happen with @SuppressWarnings is that people copy-paste entire methods, causing the suppression to come with the method, increasing the number of problems without making anything fail. In some cases I even saw @SuppressWarnings({"all","ALL"}) being shuttled around onto all kinds of methods and hiding real bugs...

So it would be nice if there were some way to say ... "any more than n violations of this rule across the whole project causes a failure, but equal or less is fine" :) Then if someone committed new violations, the build would fail. As I commit fixes, I can bring the maximum warning count down commit by commit.

This is just one idea. Of course, it doesn't stop people touching the file to increase the number, but people can turn the entire check off in the build as well and there isn't much I can do about that sort of thing. :(

@hossman
Copy link

hossman commented Apr 12, 2016

So it would be nice if there were some way to say ... "any more than n violations of this rule across the whole project causes a failure, but equal or less is fine"

Alternatively, some options to say:

  • "Write the details of all forbidden usage to a machine readable xml/json/csv report file"
  • "log all forbidden usage but don't fail the ant/maven/gradle build"

...could be combined by users to get similar effect by post-processing the report file.

I used this technique in the past with some other static code analysis tool (i can't remember which one)...

  1. Run the tool once, in fatal mode, with a rule set of things that are known to not be broken in the code base.
  2. Run the tool once, in non-fatal mode, with a rule set of things that are known to currently be broken in the code base, but you are trying to gradually fix
  3. Post process the machine readable report, and for each type of error:
    • If the number of failures of this type is less then the expected count, update the expected count (or fail the build with a useful message like "Thanks for fixing some bugs of type X! please update expected.broken.stuff.properties and reduce the expected count")
    • If the number of failures of tis type is more then the expected count, fail the build: "You increased the number of times the broken pattern of XYZ is used in the code, that is not allowed, we are phasing that out"

If the output was XML, an xslt could even be provided to transform it to emulate JUnit test results, which would make it trivial for tools like Jenkins to show the details of the failures (I can't remember which tool I saw that provided an XSLT like that out of the box, but it was really nice to have those details show up in on the Jenkins build summary page, w/o needing any custom jenkins plugins)

@uschindler
Copy link
Member

Hi,

failOnViolation can be set to false already (since 2.0) for Ant, Maven, and Gradle: http://jenkins.thetaphi.de/job/Forbidden-APIs/javadoc/check-mojo.html#failOnViolation

The idea of creating a machine-readable report is good. Unfortunately the JUnit Report format is not applicable. It is better to have a look at reports generated by CheckStyle or PMD - maybe there is a standard.

The Jenkins plugins for reports on build errors and warnings generally parse the console output (e.g. javac deprecation and rawtypes warnings, e.g., see diagram on top-right side: http://jenkins.thetaphi.de/job/Lucene-Solr-master-Linux/). Adding a parser for forbidden is easy, because it reports in emacs style (I hope it's correct emacs style, if not we might improve). Here is the documentation of the famous "Compilter warnings" plugin: https://wiki.jenkins-ci.org/display/JENKINS/Warnings+Plugin

@uschindler
Copy link
Member

Here is the Jenkins screen to configure a parser for "warnings":
image

@uschindler uschindler self-assigned this Apr 12, 2016
@hakanai
Copy link
Author

hakanai commented Aug 17, 2016

I believe Jenkins' plugin can also be set to fail the build if the number of warnings exceeds a given count, which would more or less do what I want, although too many people have access to change that value for us. :(

About the only catch with the warnings plugin is that it gets confused between the warnings of different tools if they look too similar. For instance, javac and clang warnings must look identical, because all the warnings from each show up in the other as well. So all tools should produce sufficiently unique-looking output in order for that plugin to be useful.

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

No branches or pull requests

3 participants