DRAFT: Tracking of changes from 'hazendaz' which will be managed via smaller pull requests #1120
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1120 +/- ##
============================================
+ Coverage 98.27% 98.51% +0.24%
+ Complexity 565 561 -4
============================================
Files 55 55
Lines 1214 1209 -5
Branches 105 105
============================================
- Hits 1193 1191 -2
+ Misses 10 7 -3
Partials 11 11 ☔ View full report in Codecov by Sentry. |
|
This is great job to have it merged. Can you split this into smaller PR so I can easily review them one by one ? |
|
Thanks, will be maybe a week to get back to this but will break it up for you in smaller commits. |
|
keeping open for moment but splitting out into smaller PRs as noted as well as rebased against master. |
|
@damianszczepanik For now I sent as much to review as feasible for the moment. Some of what I did not send requires some of the other items to be in place. If you could review those when you have time, as merged I will rebase this branch until such time its cleared out. Its otherwise just for reference now. When done, I'll move forward and upgrade to full junit 5 jupiter here too. I did see you added dependabot, I would suggest Renovate as a much better alternative. Dependabot will still get used in some regard for security alerts but Renovate now also supports those too and dependabot will fail to find all. If not familar with Renovate, please check out this repo of mine that has a lot of PRs on it as well as a dependency issue tracker that shows what the repo has going on. Its a more advanced config setup as I'm getting ready to split that said repo between javax and jakarta namespaces so I have it running twice on 2 different branches for that longer term need. Renovate dashboard psi-probe/psi-probe#2205 and see open PRs https://github.com/psi-probe/psi-probe/pulls to get an idea on how that one works. |
|
You can find renovate in the marketplace! |
hazendaz
force-pushed
the
master
branch
2 times, most recently
from
d4e0f7c to
f2c9ab8
Compare
| @@ -8,6 +8,7 @@ on: | |||
|
|
|||
| jobs: | |||
| build: | |||
| if: github.repository_owner == 'damianszczepanik' | |||
There was a problem hiding this comment.
I like this one, can you move it to separate branch and merge with comment d6281b4 so we can test it?
hazendaz
force-pushed
the
master
branch
2 times, most recently
from
143ee09 to
1633776
Compare
| @@ -36,7 +36,7 @@ public void getWebPage_ReturnsFailureReportFileName() { | |||
| } | |||
|
|
|||
| @Test | |||
| public void prepareReport_AddsCustomProperties() { | |||
| void prepareReport_AddsCustomProperties() { | |||
There was a problem hiding this comment.
why public is unwanted for every signature ?
There was a problem hiding this comment.
See from junit 5 comment on stack overflow https://stackoverflow.com/questions/55215949/why-junit-5-default-access-modifier-changed-to-package-private
All the various scanners at this point flag these. For example, I use sonarlint, it flags most of it, it misses a few that are also unnecessary but does for the bulk of them. Also when running tools like openrewrite, it will also do the same. As junit states, its not forced but its preferred. Back with junit 4 and before they had to have them public, it would not work otherwise, now it doesn't matter except in odd case where someone would want to see static analysis scanning complaining for not following suite with this.
There was a problem hiding this comment.
I do have to admit it is hard to get use to. I've been writing junit 5 since it was still in beta and migrated so many repos by hand that I now appreciate the reason its done this way as well as all the tools now can handle it for us when coming from older versions.
hazendaz
force-pushed
the
master
branch
3 times, most recently
from
772d96e to
2bd927b
Compare
hazendaz
changed the title
An impossible default to use a switch here is not remotely valuable as its impossible to reach without completely replacing the enum as was done in the tests to solve for non existent default throws. That hack lights up the IDE's as the class is entirely duplicated. After enough stack overflows on this then finally reading a write up from the original creator of Jmockit that it was a wrong solution, just wrote it correctly. In other words, there are 2 enums. Its either one or the other. It cannot be something else. Therefore, it doesn't need a switch statement. It should only be an 'if' statement. If it later becomes more, it could change but not seeing why now or why a about 10 year old hack suggested then contested was used here. Personally I prefer my IDE to be free of errors ;)
|
going to close this one as it was for tracking and these can start coming in normally at this point. |
In an effort to get rid of libraries coming into code bases that are long obsolete, coming here to the source for us to clear these out. This is somewhat big pull request. Each item has been constructively broken down into individual commits. All tests continue to pass. This can be further broken down if wished into smaller PRs.
General list of items
As to powermock, it is obsolete too except for its whitebox which is still extremely useful but there is no activity there. If this goes well, I don't mind upgrading this repo to junit 5 jupiter as well with mockito and replacing all the major parts of powermock as well. As to extra throws with powermock reflect, that is sort of needed here since jmockit, since I toke it over, does not yet have deencapsulation back (but that is in flight) and it should only be considered maintenance anyways. While there are other solutions out there, powermocks whitebox still seems best there and it can be isolated to just that module rather than how it is today. Other than that, I think rest would be fairly modern here, still java 8 compatible, and now shows java 21 compatibility.