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

CheckMojo: introduce failThreshold #222

Merged
merged 2 commits into from
Jun 6, 2020

Conversation

famod
Copy link

@famod famod commented May 25, 2020

Resolves #219

Notes:

  • First I cleaned up the duplicate parsing of outputFile and the scoping of bugCount and errorCount (see first commit)
  • The recent Groovy update from 3.0.3 to 3.0.4 kills your builds: https://travis-ci.org/github/spotbugs/spotbugs-maven-plugin/builds/690024559
    [ERROR] Failed to execute goal org.apache.maven.plugins:maven-invoker-plugin:3.2.1:install (prepare-integration-test) on project spotbugs-maven-plugin: Execution prepare-integration-test of goal org.apache.maven.plugins:maven-invoker-plugin:3.2.1:install failed: Plugin org.apache.maven.plugins:maven-invoker-plugin:3.2.1 or one of its dependencies could not be resolved: Could not find artifact org.testng:testng:jar:7.2.0 in google-maven-central (https://maven-central.storage-download.googleapis.com/maven2/) -> [Help 1]
    
    I get the same error locally which is why this change is not in my PR branch (based on 689c796 instead). (see first comment)
  • This PR also adds a priority prefix to the log message:
    Before:
    [ERROR] Overwritten increment in UselessAssignments.oops() [UselessAssignments] At UselessAssignments.java:[line 30] DLS_OVERWRITTEN_INCREMENT
    
    With this PR:
    [ERROR] High: Overwritten increment in UselessAssignments.oops() [UselessAssignments] At UselessAssignments.java:[line 30] DLS_OVERWRITTEN_INCREMENT
    

@famod famod force-pushed the issue-219-failThreshold branch from cbce885 to 0cea25a Compare May 25, 2020 20:26
@famod
Copy link
Author

famod commented May 25, 2020

I just realized that your Travis build uses the latest commits anyway so I rebased my branch.
This will (of course) fail all the builds for this PR until the TestNG problem is fixed.

@famod
Copy link
Author

famod commented May 26, 2020

@hazendaz WDYT?

@hazendaz
Copy link
Member

travis ci is dicey these days. I tried to rebuild this yesterday but no luck. Issue was not groovy but rather the invoker pulling testng. I don't see us using testng but it's having issues getting it. I tried running it again tonight. I'd like to see travis successful. I also need to wrap my head around the changes a bit more. A quick look seems ok but just want to review some of the naming changes and make sure it looks appropriate for current usage. So I'm on board with it just need to get travis working and look at it in a little more depth. I'll try to get to it in next couple of days. If travis continues to fail, can you take a look at what might be causing that? I'm pretty sure when I upgraded groovy it was fine on that pull request. I usually skip merging if travis is having issues unless its related to pulling a lib it cannot find. That could have been the case on the groovy update. I don't recall off hand.

@famod
Copy link
Author

famod commented May 27, 2020

@hazendaz

Well, the Groovy upgrade PR ran into the same problem: #221
I can reproduce it locally, so this is not a Travis problem, AFAICS.

@hazendaz
Copy link
Member

@famod Thanks. I'll take a look tonight.

@famod
Copy link
Author

famod commented May 31, 2020

@hazendaz friendly reminder! 🙂

@hazendaz
Copy link
Member

hazendaz commented Jun 2, 2020

I need to ping testng folks I think to get them to release their binary. Their github states it was released but obviously wans't in central last I checked. I kicked off another build just now. If it fails again, I'll reach out to that team.

@famod
Copy link
Author

famod commented Jun 2, 2020

So it failed again.

IMHO, the groovy update should be rolled back for now.

@hazendaz
Copy link
Member

hazendaz commented Jun 6, 2020

@famod Rebuilding now. Issue fixed. We don't use testng but the groovy plugin uses groovy all which we override to latest. Per groovy, they messed up by using testng on version not released to central, testng has since removed notice of testng release. Since we don't use testng, I followed groovy's work around to exclude it from the groovy plugin and builds are back to normal. As soon as this goes green, I'll review once again and then get it merged.

@hazendaz
Copy link
Member

hazendaz commented Jun 6, 2020

@famod Thank you! Merging now.

@hazendaz hazendaz merged commit 46b84f1 into spotbugs:spotbugs Jun 6, 2020
@famod
Copy link
Author

famod commented Jun 6, 2020

@hazendaz Awesome, thanks! Any plans on cutting a new release?

@famod famod deleted the issue-219-failThreshold branch June 6, 2020 21:36
@hazendaz
Copy link
Member

hazendaz commented Jun 6, 2020

@famod I'm waiting on a pull request to be corrected so the test for htm vs xml works. I'd like to include that. So hopefully soon. If I don't hear anything back this coming week on the other pull request, I'll likely just move fowards without it.

@famod
Copy link
Author

famod commented Jun 6, 2020

Thanks for your feedback!

@famod
Copy link
Author

famod commented Jun 19, 2020

@hazendaz Any news regarding a new release? Thanks!

@hazendaz
Copy link
Member

@famod I'll release this tonight.

@hazendaz
Copy link
Member

@famod Released, it will be available in central in next 2 hours or so. You should be able to otherwise pull it probably in about 15 minutes or so.

@hazendaz
Copy link
Member

Version 4.0.4 to match line of spotbugs.

@famod
Copy link
Author

famod commented Jun 20, 2020

@hazendaz Great, thanks!

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

Successfully merging this pull request may close these issues.

check goal: introduce failThreshold or similar
2 participants