-
Notifications
You must be signed in to change notification settings - Fork 642
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
Provide exception messages that occurred during stop #1435
Provide exception messages that occurred during stop #1435
Conversation
…eneric message - related to fabric8io#1251 Signed-off-by: Doyle Young <dyoung@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #1435 +/- ##
============================================
+ Coverage 58.79% 59.02% +0.22%
- Complexity 1974 1981 +7
============================================
Files 162 162
Lines 9014 9018 +4
Branches 1362 1363 +1
============================================
+ Hits 5300 5323 +23
+ Misses 3229 3205 -24
- Partials 485 490 +5
|
runService.stopStartedContainers(false, true, true, testLabel); | ||
fail("Should have thrown exception"); | ||
} catch (DockerAccessException | ExecException e) { | ||
assertEquals(e.getLocalizedMessage(), "(TEST two,TEST one)"); |
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.
nit, these arguments should be in opposite order: assertEquals(expected, actual)
|
||
runService = new RunService(docker, queryService, tracker, logOutputSpecFactory, log); | ||
|
||
try { |
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.
I think maybe we can improve this by using assertThrows
method like this:
Exception thrownException = assertThrows(DockerAccessException.class, () -> runService.stopStartedContainers(false, true, true, testLabel));
assertEquals("(TEST two,TEST one)", thrownException.getLocalizedMessage());
WDYT?
@doyleyoung: Thanks a lot for your PR 👍 . It looks good to be merged, I just have minor comment regarding the test. |
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.
beside the minor nit, looks good to me. Thanks a lot !
…aven-plugin into list_shutdown_errors
@rohanKanojia @rhuss updated the test. Thanks for the awesome project 👍 |
Provide exception messages that occurred during stop, rather than generic message - related to #1251
Signed-off-by: Doyle Young dyoung@gmail.com