run: add optional exit-status test

[edsantiago]

If run is invoked with '=N' or '!' as its
first argument, it will check the exit code
of the invoked command and fail if it does
not match the requirement (exact N for =N,
or any nonzero for '!').

Additionally, run now logs each command invoked
as well as its entire output. This will be invisible
when tests pass, but will be shown on test failure
and may offer crucial context for someone analyzing
a failed run.

Signed-off-by: Ed Santiago santiago@redhat.com

• I have reviewed the Contributor Guidelines.
• I have reviewed the Code of Conduct and agree to abide by it

[edsantiago]

Sincere apologies for the churn; I was obviously not as well prepared as I thought I was.

CI is now green. I would appreciate any feedback, whether big-picture or details.

FWIW we've been using a (narrower-focused) variant of this code on our containers projects, and the enhanced logging makes it soooo much easier to track down bugs, often with just a glance at the BATS results.

[kolyshkin]

This looks very interesting!

I have yet to review it in more details, for now what I see is

1. Instead of introducing an env var changing the behavior of run, we can add brun, erun, or run2, or chk, or something, keeping the original intact. This will actually help to smoothen the transition to the new functionality -- one can leave old test cases with run, and write the new ones using the enhanced *run* (whatever we call it). After some time, we can make legacy run emit a warning that it is obsoleted and users should switch to erun.

2. expect is not a good name as it collides with a well-known expect tool. This def needs to be renamed.

3. Does expect only supports the whole string match? I have many cases with substring match (as in [[ "$output" == *"substring"* ]] and a few with a regex match (as in [[ "$output" ~= [Rr]egexp*$ ]]. Would be nice to cover those in some way, too. I guess numeric comparisons ([ "$output" -lt 5 ], [[ "$output" <= 99 ]]) are also not uncommon.

[edsantiago]

re: nonconflicting run name - wow, flashback. In my original attempt, over a year ago, I considered that -- but can no longer remember why I dropped it and went with the envariable override. It might've simply been that I couldn't think of a suitable name. I do think it would make for much more readable code and fewer unexpected snags, so yes, I'll give that a lot of thought. Thank you.

re: expect - does anyone still use it? I used it heavily in the 80s and 90s, but haven't heard it mentioned even once in any of my programming communities in over ten years. Even if it's still in use (and I hope it is, because it's such a neat tool), is there likely to be any overlap of tcl-expect being used inside a BATS script?

re: whole-string matches - my bad, I should've spent more time documenting it. Yes, it should be possible to use any operator, including the nonexistent !~. See one of the newly-added tests for examples.

Thank you for your feedback! Coming up with good names is difficult for me, but I promise to give it a lot of thought over the next day or two.

[martin-schulze-vireso]

I think most of the supported use cases in this PR are already implemented. As far as I see, there are three main features:

1. fail when the command fails
2. print the output (only) when the command fails
3. check if the output matches

We already know when not to use run and have bats-assert which gives us:

1. every command that is not started via run will fail the test if it returns non zero
2. every command that is not started via run will show its outputs to stdout/stderr in the log when the test fails
3. bats-assert already offers many output checks in concert with run and only prints the output on failure

The advantages with the approach in this PR seem to be:

• no additional library for the assertion (-> we could do that with bats-assert too)
• less noise in the log compared to execution without run (-> bats-assert+run deals with that)
• checking the status implicitly while printing the output on failure

Did I miss something? Only the 3rd point seems to be lacking right now.

[edsantiago]

@martin-schulze-vireso thank you, I was not acquainted with bats-assert. I'll be taking a close look at it soon.

[kolyshkin]

Some more thoughts on run in its current form, and the proposed improvements to those who'll be reviewing this.

After reading and fixing lots of bats code using run, I have submitted #343 to better document it (and its various pitfalls), and I think I nailed it in #366. The gist of the latter it is

• do not use run unless you want to check both non-zero exit status and the output

as in all the other cases not using run will result in a simpler/better code. I won't duplicate the examples here, please see https://github.com/kolyshkin/bats-core/blob/patch-2/README.md#run-test-other-commands (make sure to also scroll down to "when not to use run").

Now, if users follow this advice, the result is OK, but it's not uniform. In some cases run is used, in some it's not. In some cases error checking is implicit (through set -e), in some cases it's explicit ([ "$status" -eq 42 ]).

Here is a not-so-good example when run is used everywhere (I've seen lots of such code recently):

@test foo {
   run cmd start something ...
   [ "$status" -eq 0 ]
   id="$output"

   run cmd exec "$id" something ...
   echo "cmd exec something: $output"
   [ "$status" -eq 0 ]
   [[ "$output" == *"success"* ]]

   run cmd exec "$id"_bad something ...
   [ "$status" -ne 0 ]
   [[ "$output" == *"invalid id"* ]]

   run cmd exec "$id" false
   [ "$status" -ne 0 ]

The problem is, it is too verbose. To fix this, it can be rewritten to

@test foo {
   id=$(cmd start something ...)

   output=$(cmd exec "$id" something ...)
   echo "cmd exec something: $output"
   [[ "$output" == *"success"* ]]

   run cmd exec "$id"_bad something ...
   [ "$status" -ne 0 ]
   [[ "$output" == *"invalid id"* ]]

   ! cmd exec "$id" false

This is better, but have the following issues:

• run is not used uniformly, and it is not too obvious why (unless you know the "when to use run" rule above);
• we still need echo to show the output in case we need to collect it;
• this echo has to duplicate the command and its arguments;
• when the run is used, it's still too verbose.

The new proposed run (let's call it erun for now) apparently solves all this:

@test foo {
   erun cmd start something ... # implicit zero status check
   id="$output"

   erun cmd exec "$id" something ... # implicit logging, too!
   [[ "$output" == *"success"* ]]

   erun '!' cmd exec "$id"_bad something ...
   [[ "$output" == *"invalid id"* ]]

   erun '!' cmd exec "$id" false    

Now this is both uniform and compact (note I'm using '!' to signify I want to check that cmd exited with non-zero), plus errors are (presumably) reported better.

Finally, if we add the proposed expect to the list, we can improve error reporting for mismatched output.

Hope that all make sense.

[martin-schulze-vireso]

Now, if users follow this advice, the result is OK, but it's not uniform.

Okay, this is what I did not notice above and it all makes sense now.

I think we should separate the discussion into two parts: The output checking and the return code checks. bats-assert offers solutions for both, which rely on run.

I think the output checking will always require an additional command, or the *run statement would become very convoluted. Therefore, the current bats-assert solution is close to optimal in this regard.

However, the return code checks are more interesting for this PR. I think the proposed solution of an exit code checked run command that offers the same output parsing interface to be compatible with existing assert_output solutions would enhance readability a lot. It would suffice to allow for exit code =0, !=0 (and maybe =X) to solve 99% of cases and the rest should be dealt with via explicit checking of $status.

To stay with the naming above, I would propose the following terse syntax:

erun exit 0 # succeeds (expects return code 0, got 0)
erun exit 1 # fails (expected return code 0, got 1)
erun ! exit 1 # succeeds (expected non zero exit code)
erun !2 exit 1 # fails (expects exit code 2)

[edsantiago]

Thank you both for the suggestions and encouragement. I haven't played with bats-assert yet because of installation and come-up-to-speed-itis, but I did take a few moments to adopt your ideas. In particular:

• environment variable dependency is gone. New function is runv - is that ok? It's slightly easier to type, the 'v' can stand for 'verbose' and/or 'validation' which (maybe?) makes it more memorable.
• bare number as first argument is gone; expectation is now '!', or '!N', or '?' (or, if none, zero). I'm not sure how I feel about this: runv 1 false looks so much cleaner than runv '!1' false. Would anyone object to my adding bare-number usage back?
• expect is now xpect, and I added more usage examples to README-runv. One reason I've appreciated my expect helper, which I'd like to see if bats-assert can do, is that mine prints actual/expect on separate lines, making it so easy to catch single-character errors. But I can't really comment because I haven't yet dived into bats-assert. That may be a weekend task.

[martin-schulze-vireso]

While working on #368 I noticed that the current debug trap tracing can be problematic with the run function. This might hit you as well, so quick heads up: please add a test that verifies the correct error location being reported for your new run function.

[edsantiago]

quick heads up: please add a test that verifies the correct error location being reported

Yikes. Thank you; seems like my best course is to wait for #368 to merge, then rebase on it.

[edsantiago]

I've pushed one more commit; this time converging on xrun and xpect. Not quite as easy on the eyes as runv, IMO, but the alliteration makes them memorable and (I hope) easier to use & read.

And, I had a chance to use bats-assert, and see much to like in it, but there are many cases where I think xpect produces output that can save someone a lot of time. In particular, the (admittedly optional) test description can give someone an immediate insight into what went wrong:

 xrun seq 10001 10005
 xpect "${#lines[*]}" = 6 "Number of output lines"   | assert [ "${#lines[*]}" = "6" ]
 #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv     |  -- assertion failed --
 #|     FAIL: Number of output lines                 |  expression : [ 5 = 6 ]
 #| expected: '6'                                    |  --
 #|   actual: '5'                                    |
 #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^     |

I also like the vertical layout:

 expect="The quick brown fox jumped over the lazy dog"
 xrun echo "The quick brown fox jumper over the lazy dog"
 xpect "$expect" "Comparing fail layout on long string"
 #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
 #|     FAIL: Comparing fail layout on long string
 #| expected: 'The quick brown fox jumped over the lazy dog'
 #|   actual: 'The quick brown fox jumper over the lazy dog'
 #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

vs

 assert [ "$output" = "$expect" ]
 -- assertion failed --
 expression : [ The quick brown fox jumper over the lazy dog = The quick brown fox jumped over the lazy dog ]
 --

In the world I code in, too often I have to hand-edit an error log to put expected vs actual on top of each other, to see the actual differences.

[martin-schulze-vireso]

quick heads up: please add a test that verifies the correct error location being reported

Yikes. Thank you; seems like my best course is to wait for #368 to merge, then rebase on it.

I did not look into it that deep but I see two ways this can play out: Either your code already works as intended, or rebasing won't be enough. I had to do special case handling with aborting during run. Now that I come do think of it, I probably would have to handle each internal function as a special case, when it is aborted via CTRL+C, it's just that the run functions are more likely to hang/be aborted.

assert [ "$output" = "$expect" ]

I think you should look into assert_equal for a fair comparison. I can highly recommend the readme at https://github.com/bats-core/bats-assert to see a list of all supported tests.

Regarding the naming: When we are okay with using the first parameter to indicate return code checks, we could simply upgrade the run function. I don't think there are many users who run a ? or ! command right now, so I don't see clashes. I am not sure how much this would cost performance wise but I think it would be much cleaner to have only one run function. (Also thinking about the special case handling above.)

[edsantiago]

I've addressed the wrong-line-number issue, although I'm not sure I've done so in the best possible way. I've left it as a separate commit for ease of review.

we could simply upgrade the run function

That would prevent the usage I most like: xrun command with implicit zero-exit check. I think requiring test writers to remember to write run '!0' command on each invocation is a heavy burden, which means it won't be done. Remembering to type xrun isn't much better, but I think it's a slightly easier habit to form. I really don't know what the big world out there prefers, though.

[martin-schulze-vireso]

That would prevent the usage I most like: xrun command with implicit zero-exit check [...] I really don't know what the big world out there prefers, though.

Well, I don't know either but I added a rationale to the other issue (#377 (comment)).

However, I agree that return code checks should be default. In a sense we already have this situation by having "normal" commands (without run) be return code checked, so using run explicitly disables this check. However, the way many people seem to think of bats tests is: you have to run every command you want to check.

I think requiring test writers to remember to write run '!0' command on each invocation is a heavy burden

I was a bit unclear. I think we should have something like:

• run <command> -> current solution, ignores return code
• run ? <command> -> expect success, this is equivalent to <command> (except unconditionally printing the output)
• run ! <command> -> expect failure
• run =<return code> <command> -> expect return code
• run !=<return code> <command> -> expect not return code

The advantage over a separate function is the unified control flow, which reduces special case handling.

I'd also like @kolyshkin and @sublimino to weigh in on this.

Apart from that: Did you look into the asserts library? (Especially assert_output and assert_lines) I feel you are replicating much of its work and the old title already showed that this is in fact two PRs, one for xrun and one for xpect, which I would like to unbundle.

[edsantiago]

Nice. I like the =N form, that's short and memorable and friendly. I like ! also, but am not sure I understand the reasoning behind ? or !=N. Could you elaborate on how/why you see these being used?

This has grown somewhat beyond the scope of my original submission, so I've prepared a separate local branch with just the changes to run as described in your last comment (except for ? and !=, pending further exploration), and with no xpect because that clearly merits a separate discussion. I'm leaning toward submitting it as a separate PR and closing this one, because it's so different, but that loses this conversation. What do you recommend?

[NorseGaud]

@edsantiago check out #467. I think that will be a useful feature for you.

[martin-schulze-vireso]

I rebased onto current master and resolved the conflicts. I also restructured the test code and removed the tracing parts that might go into #467 and _bats_die together with it's verbose output. I hope I did not break anything.

[martin-schulze-vireso]

One thing that is still missing is the printing of $output. I removed that because it is intended for the trace feature. However, I find the usecase of printing the output on error quite compelling. So when we don't check the exit code, run should behave as before and remain silent. When we do exit code checking and the check fails, I'd propose to print output.

[martin-schulze-vireso]

@edsantiago Do you mind reviewing if I broke some of your intentions?

@bats-core/bats-core I'd also like the other maintainers' opinions on this.

[edsantiago]

@martin-schulze-vireso thank you for taking this on again. Looks good at first pass, but by now this is different enough from my original PR that I'm OK with you closing and resubmitting as yours.

[martin-schulze-vireso]

@edsantiago I am okay with sharing the fame for your ground work if you are okay with sharing the git blame. ;)