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

Provide exception messages that occurred during stop #1435

Merged
merged 5 commits into from Mar 8, 2021

Conversation

doyleyoung
Copy link
Contributor

Provide exception messages that occurred during stop, rather than generic message - related to #1251

Signed-off-by: Doyle Young dyoung@gmail.com

…eneric message - related to fabric8io#1251

Signed-off-by: Doyle Young <dyoung@gmail.com>
@codecov
Copy link

codecov bot commented Feb 15, 2021

Codecov Report

Merging #1435 (794271a) into master (9f729e2) will increase coverage by 0.22%.
The diff coverage is 100.00%.

@@             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     
Impacted Files Coverage Δ Complexity Δ
...va/io/fabric8/maven/docker/service/RunService.java 62.50% <100.00%> (+7.27%) 40.00 <0.00> (+6.00)
...ic8/maven/docker/access/DockerAccessException.java 63.63% <0.00%> (+9.09%) 4.00% <0.00%> (+1.00%)

@rohanKanojia rohanKanojia added this to the 0.35.0 milestone Mar 6, 2021
runService.stopStartedContainers(false, true, true, testLabel);
fail("Should have thrown exception");
} catch (DockerAccessException | ExecException e) {
assertEquals(e.getLocalizedMessage(), "(TEST two,TEST one)");
Copy link
Member

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 {
Copy link
Member

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?

@rohanKanojia
Copy link
Member

@doyleyoung: Thanks a lot for your PR 👍 . It looks good to be merged, I just have minor comment regarding the test.

Copy link
Collaborator

@rhuss rhuss left a 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 !

@doyleyoung
Copy link
Contributor Author

@rohanKanojia @rhuss updated the test. Thanks for the awesome project 👍

@rohanKanojia rohanKanojia merged commit b2f287d into fabric8io:master Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants