-
Notifications
You must be signed in to change notification settings - Fork 174
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
7432: Update the spotbugs maven plugin #324
Conversation
👋 Welcome back reinhapa! A progress list of the required criteria for merging this PR into |
Webrevs
|
It doesn't look like this as trivial an upgrade as the other dependencies, something must have been updated on the dependency side that is finding new potential bugs in the JMC code. At the moment there are 3 issues with jmc.ui.common: https://github.com/openjdk/jmc/runs/3964924803?check_suite_focus=true#step:7:7015 The code either has to be adjusted, or if appropriate can be added to the spotbugs exclude file to filter the results when spotbugs runs: https://github.com/openjdk/jmc/blob/master/configuration/spotbugs/spotbugs-exclude.xml |
I saw it, when running it locally. It also seems to have some issues with a lot of static methods returning static class variables, that in my opinion do not seem to be a problem as those There seem to be already an issue opened on the spotbugs project. I will still check the other issues and include potential class changes within this PR later next week. |
Have you had time to take a look at the failing tests yet @reinhapa? |
@thegreystone unfortunately not, actually swamped in work 🙁, hope it get's better this week... |
Error:
Bug description: https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#dmi-random-object-created-and-used-only-once-dmi-random-used-only-once TwitterOAuthHeaderGenerator @ line 188: https://github.com/openjdk/jmc/blob/master/application/org.openjdk.jmc.console.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterOAuthHeaderGenerator.java#L188 It looks to be complaining that we're creating a new Random object at line 186 everytime |
@aptmac I already got a working version here locally but I'm not yet happy with it... Seems that storing the Random in an instance variable does not help. The only way things pass at the moment is using a |
21a20fb
to
3b2b45d
Compare
...ightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/overview/ResultReportUi.java
Show resolved
Hide resolved
Signed-off-by: Patrick Reinhart <patrick@reini.net>
@reinhapa This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit ae37757. |
Signed-off-by: Patrick Reinhart patrick@reini.net
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jmc pull/324/head:pull/324
$ git checkout pull/324
Update a local copy of the PR:
$ git checkout pull/324
$ git pull https://git.openjdk.java.net/jmc pull/324/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 324
View PR using the GUI difftool:
$ git pr show -t 324
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jmc/pull/324.diff