Should we use run =X?

[martin-schulze-vireso]

After landing #367 and rebasing #477 onto it, I got a host of shellcheck warnings for the new run =<N> syntax along the lines of:

  run =0   true
      ^-- SC2283: Remove spaces around = to assign (or use [ ] to compare, or quote '=' if literal).

My decision to use = was that it is concise and I wanted to avoid run '=0'. However, seeing this warning made me acutely aware of the ramifications. Missing the space would assign run=0 and works without error because the next part is a command that should be runnable.

I see following ways going forward:

1. expect run '=0'
2. find another syntax for the return code check (I checked run ?0 and run ==0, which also give shellcheck warnings but at least would not fail silently when omitting the space)
3. stay with run =0 and update shellcheck to balk at run=0 and don't balk at run =0 in bats tests

Pinging @edsantiago since you floated other ideas in the original PR.

I am giving this some urgency since I want to settle the issue before run =<N> gets published in the next release.

[edsantiago]

I still like just plain run X. It's what we use in our internal BATS environment. I have never run into a situation where X (0-127) is a valid command.

[martin-schulze-vireso]

Thanks for the feedback @edsantiago. What about the other options? Do you find run '=0' true too verbose?

IMHO, run 0 true is slightly less newcomer friendly than run =0 true.

[edsantiago]

Are you asking about zero specifically, or any exit code?

Zero-specific: In our environment, 0 is assumed. run foo means if foo exits nonzero, fail. (I realize this is incompatible with historical BATS).

N-general: I'll admit it takes a little getting used to run N command, but by now we're all used to it. I understand the need to balance newbie-friendly with useful-to-experts, though.

[martin-schulze-vireso]

Are you asking about zero specifically, or any exit code?

Any form of exit code.

Just to put up some other ideas:

• run --=N: run -=N is shorter but looks like arithmetic
• run --?N run ?N: trigger SC2035: Use ./*glob* or -- *glob* so names with dashes won't become options; run ?N has the advantage of not accidentally becoming a NOOP when leaving out the space, however the globbing is a problem
• run --expect N run --status=N or run --status==N: these are pretty verbose (but still shorter than [ $status -eq N ]

[jasonkarns]

I'm only just getting up to speed on the thread of related issues, but I feel like I'm missing the point. Why are we "upgrading" the run command to be doing more than one job?

I believe its original purpose was to run a command in such a way that the results (output and exit status) can be inspected. having it also take in expected exit status codes, it's now doing two jobs: invoking the subject under test and also doing assertions.

It seems contrary to most testing frameworks for the "act" step (in AAA parlance) to have built-in assertions. I think the problem we're having with how to cleanly and clearly provide an expected status code is a side effect of trying to have run take part in both the Act and Assert steps when it should not.

[martin-schulze-vireso]

As I see it, this is the convergence of multiple efforts:

1. we want to have more batteries included, i.e. there was a discussion about merging bats-assert into core. I am not sure if the whole library should be treated this way, but exit code checking is so fundamental, that it should be one of the first to be moved.
2. the documentation efforts around when not to use run showed a complexity that does not lend itself to beginners. It would be more understandable to have a single entry point with explicit customization.
3. the existing exit code checks, even those relying on assert_success, don't communicate the command that failed. By letting run itself fail, we get that out of the standard error message.

I believe its original purpose was to run a command in such a way that the results (output and exit status) can be inspected. having it also take in expected exit status codes, it's now doing two jobs: invoking the subject under test and also doing assertions.

If the preexisting code is still working, I don't see an issue with adding functionality. However, I must admit that mixing two responsibilities might not be the best solution. Yet, I think that point 3 from above should merit some consideration. There is a PR in the making for storing the command that was run. Maybe assert_success and the other checks in bats-assert could use that as additional context?

It seems contrary to most testing frameworks for the "act" step (in AAA parlance) to have built-in assertions.

Then we're already in trouble due to set -e which is very similar to many other languages "feature" of failing tests on exceptions. My standard for evaluating this would be: does it make the test code more or less clear?
=N is less visible than assert_success but not by much and its much clearer than set -e's "magic" which the current "when not to use run" section advocates for in some circumstances.

I think the problem we're having with how to cleanly and clearly provide an expected status code is a side effect of trying to have run take part in both the Act and Assert steps when it should not.

I think the two are only related in so far as the more explicit options bloat an already long command. Admittedly, this is not a problem for assert_success.

As I said above, what do you think of moving assert_success/failure into core and adding the failing command as additional context? @jasonkarns

[kolyshkin]

I'd rather have this fixed this in shellcheck, if at all possible, as run =N cmd syntax looks way more readable than run N cmd.

[martin-schulze-vireso]

@kolyshkin Patching this in shellcheck will be a steep uphill battle. The checks might be deeply ingrained and shellcheck is written in Haskell... I already made a PR for transitioning to the run =<N> notation that took me half a week and I got lucky with finding a template, because it was similar to another check.

What bothers me most with the shellcheck venue is that it is optional. Since bats syntax is a relatively recent addition, there will be many projects around that don't use it because they either don't know or cannot migrate because their suite is too big and full of warnings.

Currently, I am tending towards run -<N>, which would (hopefully?) break loudly when it becomes run-<N>, triggers no shellcheck error and is somewhat readable. I also like the syntax highlighting when writing run '=<N>' but there will definitely be people who omit the quotes and will get bitten. Then again, I am not sure how one could write run=<N> command and fail to notice that run's expected outputs are wrong. However, finding the problem might become a hassle.

Apart from that I'd also like your input on the AAA debate: Should we avoid checking the return code in run?

[martin-schulze-vireso]

Time is running up on the 1.5.0 release and we should resolve this before the release.

@kolyshkin @edsantiago What do you think of my run -<N> suggestion above?

@jasonkarns I assume you are still opposed on grounds of AAA but you did not reply to my answer comment.

[edsantiago]

I actually find run -N to be less readable than run N, but perhaps my brain has adjusted over the many years I've been using the latter... so my opinion can't count for much. The shellcheck situation is really unfortunate because run =N is by far the best option.

[martin-schulze-vireso]

I think shellcheck is right to complain about this. Carving out an exception for run= in bats scripts does not seem worth it for me. I find the lack of common prefix annyoing to parse with the run <n> version.

I only dislike two things about -<n>: the association to negative values and that it is not indicative of a comparison.

[martin-schulze-vireso]

Since there was no further feedback I am going down the -N route. Maybe we'll find something better later but this should be good enough for now and should be easily maintainable for longterm backwards-compatibility.