RFC: rlAssert terminate test script immediately

[atodorov]

Hello beaker-devs,
in lorax-composer we would like to have rlAssert statements (and anything else that can report FAIL really) to terminate the test script immediately. It is fine with us if this is not enabled by default but we need some way to make it happen.

Can you point me in the right direction? Where do I look internally in beakerlib to see if we can hack something up? Ideally an ENV variable or a config file will work great.

CC @larskarlitski

[hegerj]

Such behaviour should not be hard to implement.
There is an internal counter for failed asserts in journal.sh called $__INTERNAL_TEST_STATE. You could make the test exit when the counter is incremented (in function rljAddTest()). Only behave this way when ENV variable is set is also easy to do.

However what about Cleanup? If you exited the test after first failed assert (for example by calling rlPhaseEnd; rlJournalEnd) but the test would just end and there would be no cleanup after. Is this fine by you?

btw we are beakerlib developers, not beaker developers :)

[AloisMahdal]

Just my $.02; what I'd normally do is create rlAssert/rlRun/etc. wrapper like:

rlassert_wrap() {
    local old new
    local wrapped=${FUNCNAME[1]}    # not sure about the index ;-)
    rlGetPhaseState; old=$?         # this works only for up to 255
                                    # fails .. but that should not
                                    # be possible with your policy
                                    # in place
    "$wrapped" "$@"; rlGetPhaseState; new=$?
    test $new -gt $old && {
        rlLogDebug "failed assert; giving up: $wrapped()"
        # save some extra details if you want... 
        rlPhaseEnd 
        rlJournalEnd
        exit 3
    }
}

then you can call this as

rlassert_wrap rlRun "true"
rlassert_wrap rlRun "false"
rlassert_wrap rlRun "true"    # will never run

of course you might want to put much less or much more in the fail handler; depending on your situation.

If you need to avoid touching your current test code, then you could do this eg. by injecting own beakerlib.sh that overloads all original functions:

rlRun() { rlassert_wrap rlRun "$@"; }
rlPass() { rlassert_wrap rlPass "$@"; }
...

[sopos]

Simple set -e might be used too. Exit signal can be caught by trap handler so even the clean up could be done this way.

[sopos]

If you want to limit it just to all asserts (everything what generates :: [ FAIL ] ::), then you could inject some code for handling it into __INTERNAL_LogAndJournalFail. This is the only function where all the failures meet in one place.
However one should not rely on __INTERNAL functions, thus rljAddTest could be used but the code would need to be more complex. Therefore I would go for the __INTERNAL_LogAndJournalFail.

[AloisMahdal]

Well problem with set -e is that you can't use it for foreign code because it might rely on this not being set. (I.e. there could be something that exits with non-zero somewhere.) Barring that, you'd still have to make sure you restore the shellopts, which is not easy to do in a robust way.

Problem with __INTERNAL* is just as you said, @sopos; one just should never ever use it. Relying on non-public API is just outright bad idea.

I don't know anything about rljAddTest (couldn't find it in beakerlib(1)).

[larskarlitski]

Thanks for these ideas! I've tried monkey-patching __INTERNAL_LogAndJournalFail and it seems to work well: https://github.com/weldr/lorax/blob/e78fd46f0a37f0152f880e907bf5f06e82f9aca7/tests/cli/lib/lib.sh#L5

Except of course that it will fail when beakerlib changes. I'd really like to see something like this as a proper feature. Would you be interested in that?

Some context: when debugging a test failure, it's really useful to have a machine stay in exactly the state it's in when the assert happens. Our test scripts take a long time to run and usually clean up after themselves, which makes it harder to find out what exactly went wrong.

[AloisMahdal]

I sympathise with the importance of preserving the state. But:

1. You can solve it with wrappers, as I've shown.
2. even if Beakerlib supported it, its functions can only exit the process they're running, so it won't work if the assert failure happens within subprocess or subshell. Well unless you design your code with this failure logic in mind (ie. enforce proper use of return and maybe keep the logic simple & flat).

Hence I would not support this on Beakerlib level; I'd rather see another library around it that helps with test code flow, which is cleaner and much more powerful.

(BTW, is exit what you really want? Isn't eg. waiting for interaction better? You could actually preserve also environment vars, plus it does not suffer from the problem with subshells.)

[larskarlitski]

I sympathise with the importance of preserving the state. But:

1. You can solve it with wrappers, as I've shown.

Indeed, but I'd have to wrap every function that might fail. If I miss one or beakerlib adds new ones in the future, my tests will break in subtle ways.

2. even if Beakerlib supported it, its functions can only exit the process they're running, so it won't work if the assert failure happens within subprocess or subshell.  Well unless you design your code with this failure logic in mind (ie. enforce proper use of `return` and maybe keep the logic simple & flat).

Fair enough. I didn't know beakerlib supports running like this. That makes having such a feature effectively impossible.

Hence I would not support this on Beakerlib level; I'd rather see another library around it that helps with test code flow, which is cleaner and much more powerful.

(BTW, is exit what you really want? Isn't eg. waiting for interaction better? You could actually preserve also environment vars, plus it does not suffer from the problem with subshells.)

We're running the test script through ssh on a virtual machine. The surrounding harness leaves the machine running when the test script exits with non-0. Good idea about preserving the environment. We'd need to find a way to signal that that's the state the script is in, but it's definitely something to think about. Thanks!

Feel free to close this bug.

[AloisMahdal]

Indeed, but I'd have to wrap every function that might fail. If I miss one or beakerlib adds new ones in the future, my tests will break in subtle ways.

Only if your tests use the new functions.

Feel free to close this bug.

I'm not myself so strictly against this feature; I see the usefulness. To summarize my argument: what you want is flow control, while most core features in BeakerLib are about logging and setup/cleanup (plus some fact collecton utilities on top); so I would be careful of trying to extend it that direction (esp. if it might not be so reliable after all).

[sopos]

As mentioned above. Beakerlib cannot reliably catch all the possibilities. Thus direct support does not make sense. To be more user friendly and allow user to better handle it on the user's side I would be willing to make some public function similar to __INTERNAL_LogAndJournalFail which would serve as a single point for user to create its hooks.

[sopos]

@larskarlitski if you are still interested in this feature, please propose a PR. As a control variable I would propose BEAKERLIB_EXIT_ON_FAIL, if set to 1 the exit would be in place. Note, it would deserve also extending the documentation.

Otherwise I'm going to close this issue.

[larskarlitski]

Thanks for coming back to this. I don't have an immediate need for it at the moment.