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

teardown_suite does not run when setup_suite fails #43

Closed
n0vember opened this issue Sep 5, 2017 · 8 comments
Closed

teardown_suite does not run when setup_suite fails #43

n0vember opened this issue Sep 5, 2017 · 8 comments

Comments

@n0vember
Copy link
Contributor

n0vember commented Sep 5, 2017

When for some reason setup_suite fails, bash_unit stops and teardown_suite is never played. Which is unfortunate, because I expect teardown_suite to clean-up things in order to launch the tests again.

@pgrange
Copy link
Owner

pgrange commented Nov 10, 2017

Hi,
After our IRL conversation about this issue, would you mind elaborate on the problem ? May be with actual code and where it breaks and why it is a problem that teardown_suite is not executed ?

Regards,

@n0vember
Copy link
Contributor Author

Example is simple : I use bash_unit to test code that builds containers or virtual machines and configures them. My setup_suite code first builds the containers or machines, then plays an ansible playbook on them. Tests then check various things on what have been done. Teardown_suite destroys the containers/machines. If ansible run fails, everything is stopped. Teardown_suite is never played. The containers/machines are still running when they should be destroyed.

My actual workaround is to set a trap in setup_suite to destroy the machines over SIGTERM. So whenever and for whatever reason bash_unit ends, my machines are destroyed. But it is not clean not to use teardown_suite for that.

@saez0pub
Copy link

Any update on this issue ?

@pgrange
Copy link
Owner

pgrange commented May 9, 2019

Nope, no update. Truth being told I am not 100% sure of the implications of this change. I think it is quite odd to try to teardown something which has not been setup.

But any merge-request would be appreciated and evaluated with kindness.

@camswords
Copy link

camswords commented May 13, 2021

We've encountered the same issue. My bash skill isn't really up to the task of submitting an MR, but here is an example of the problem if it helps anyone else.

Containers run in setup_suite are not cleaned up when a test fails because teardown_suite does not run, which means I have to manually stop the container/network prior to running the test again.

@pgrange
Copy link
Owner

pgrange commented May 15, 2021

Hello @camswords and thank you for using bash_unit.

Pull request #73 should fix this. You can check if it works for you by using branch teardown_anyway

Note that, with this version, you must not use any bash_unit assertion in setup_suite or use exit in setup_suite for teardown_suite to be run. If you do, the shell will exit and nothing will be executed after that. Looking at the code you shared before, I think it will not be an issue for your use case as you don't do that in your setup_suite.

This constraint is related to the way bash treats exit and the fact that bash_unit uses exit to terminate test execution.

An alternative implementation, which would allow the use of exit in setup_suite would have been to run setup_suite in a sub-shell but in this case, this would forbid us to define variables in setup_suite and use them in tests or, at least, in teardown_suite. I think this would be more tricky to understand and I prefer to avoid that.

I still need to improve documentation to explicitly state that one should not use bash_unit assertions in setup_suite. I don't know @n0vember if this would work for you too.

Regards,

@camswords
Copy link

I've tested the teardown_anyway branch, and it solves the problem we're having 🙇. I think having no exit statements or assertions in the setup_suite is a reasonable expectation 👍

@pgrange pgrange closed this as completed in 6acb51b Jun 1, 2021
pgrange added a commit that referenced this issue Jun 1, 2021
@pgrange
Copy link
Owner

pgrange commented Jun 1, 2021

Thanks for your feedback @camswords. I've merged into master and released version 1.7.2 of bash_unit with this patch.

By the way, I can't help but notice that you don't use assert_fail in this code.

The assertion looks like this:

grep -q "wrongpassword" output/test_railsgoat_authenticated_scan.log
assert_not_equals "0" "$?" "Password present in logs"

Where it could have been that:

assert_fail \
    "grep -q wrongpassword output/test_railsgoat_authenticated_scan.log" \
    "Password present in logs"

If you want to share the rationale behind that I would be happy to hear, just in case there is food for an improvement in bash_unit assertion.

Regards,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants