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

[skip ci] Make it a fatal error if vic machine fails to cleanup a VCH #6980

Merged
merged 1 commit into from
Mar 19, 2018

Conversation

mhagen-vmware
Copy link
Contributor

@mhagen-vmware mhagen-vmware commented Dec 19, 2017

fixes #7040
Also needs #7060 to be merged to complete

@@ -322,7 +322,8 @@ Run Secret VIC Machine Inspect Command

Run VIC Machine Delete Command
${rc} ${output}= Run Secret VIC Machine Delete Command %{VCH-NAME}
Wait Until Keyword Succeeds 6x 5s Check Delete Success %{VCH-NAME}
${status}= Run Keyword And Return Status Wait Until Keyword Succeeds 6x 5s Check Delete Success %{VCH-NAME}
Run Keyword Unless ${status} Fatal Error vic-machine delete has failed to remove the VCH: %{VCH-NAME} as expected
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way we could have a suite/group cleanup check?
In both the vic-machine-service tests and the delete-resource-pool tests we're leaking because we don't call vic-machine delete, not because it fails.

@@ -322,7 +322,8 @@ Run Secret VIC Machine Inspect Command

Run VIC Machine Delete Command
${rc} ${output}= Run Secret VIC Machine Delete Command %{VCH-NAME}
Wait Until Keyword Succeeds 6x 5s Check Delete Success %{VCH-NAME}
${status}= Run Keyword And Return Status Wait Until Keyword Succeeds 6x 5s Check Delete Success %{VCH-NAME}
Run Keyword Unless ${status} Conditional Fatal Error vic-machine delete has failed to remove the VCH: %{VCH-NAME} as expected
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a request for a different Keyword, but I would more naturally phrase this with the same imperatives used in unit tests:

  • assert
  • require

Assert is a failure, require is a fatal failure. In this case we're changing the behaviour of assert to control whether it become fatal, so it wraps both the check of status and the appropriate handling on failure into a single keyword:

${status}=  Run Keyword And Return Status  Wait Until Keyword Succeeds  6x  5s  Check Delete Success  %{VCH-NAME}
Assert  ${status}  "vic-machine delete failed to remove %{VCH-NAME}"

@sgairo
Copy link
Contributor

sgairo commented Mar 19, 2018

Should the [skip ci] be moved to the body?

@codecov-io
Copy link

codecov-io commented Mar 19, 2018

Codecov Report

Merging #6980 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6980      +/-   ##
==========================================
- Coverage   31.58%   31.57%   -0.02%     
==========================================
  Files         278      278              
  Lines       42173    42173              
==========================================
- Hits        13322    13315       -7     
- Misses      27684    27692       +8     
+ Partials     1167     1166       -1
Impacted Files Coverage Δ
lib/portlayer/attach/communication/lazy.go 68.18% <0%> (-22.73%) ⬇️
pkg/logmgr/logmgr.go 65.97% <0%> (-1.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe9540e...c159540. Read the comment docs.

@mhagen-vmware
Copy link
Contributor Author

@sgairo [skip ci] needs to be in the title still:
https://github.com/vmware/vic/blob/master/CONTRIBUTING.md#automated-testing

@mhagen-vmware mhagen-vmware merged commit 0fecfdd into master Mar 19, 2018
@mhagen-vmware mhagen-vmware deleted the feature/fatal-cleanup branch March 19, 2018 19:08
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.

Add a square bracket fast failure tag
6 participants