-
Notifications
You must be signed in to change notification settings - Fork 480
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
fixed non final variable code smell #1849
Conversation
Eclipse JKube CI ReportStarted new GH workflow run for #1849 (2022-10-16T14:35:08Z) ⚙️ JKube E2E Tests (3259749854)
|
Codecov Report
@@ Coverage Diff @@
## master #1849 +/- ##
============================================
+ Coverage 52.99% 53.00% +0.01%
- Complexity 3937 3938 +1
============================================
Files 464 464
Lines 20745 20744 -1
Branches 2809 2809
============================================
+ Hits 10993 10996 +3
+ Misses 8625 8622 -3
+ Partials 1127 1126 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Hi @manusa could you pass the PR? the CI is having some glitch |
getProgressBar was used by the first method, after deletion it too was referred as unused
Kudos, SonarCloud Quality Gate passed! |
|
||
// use port | ||
//noinspection resource | ||
ServerSocket ignored = new ServerSocket(foundPort); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement should be enclosed in a try-with-resources block so that the "ignored" port is closed when the test completes
Exception exception = assertThrows(IllegalStateException.class, | ||
() -> IoUtil.getFreeRandomPort(foundPort, foundPort, 3)); | ||
|
||
|
||
String expectedMessage = "Cannot find a free random port in the range [" + foundPort + ", " + foundPort + "] after 3 attempts"; | ||
String actualMessage = exception.getMessage(); | ||
|
||
assertTrue(actualMessage.contains(expectedMessage)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be converted to a single assertion using AssertJ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thx!
Thx for going for the extra mile, really appreciate it. I left a few comments that you can address (if you want) in a subsequent PR. |
Hi @manusa! I've fixed the issues and learned along the way (thanks for that 😉) should I create another PR? |
Description
Fixes #1815
Type of change
Checklist