rbenv shell specs- refactor to test the behavior instead of the implementation?

[richiethomas]

I'm reading through the bats tests for rbenv-sh-shell now, and I notice that many of them test that the tests assert that specific code is output via echo, cat, etc. Some even test that if conditions are printed to the screen. Example:

@test "shell revert (fish)" {
  RBENV_SHELL=fish run rbenv-sh-shell -
  assert_success
  assert_line 0 'if set -q RBENV_VERSION_OLD'
}

This was initially confusing to me as a newbie to the codebase, and it was only later that I landed on the hypothesis that this is because that code will be evaluated by the rbenv shell function (correct?). If that's the case, isn't this equivalent to testing the implementation instead of testing the behavior? I would think this would require someone attempting a green-to-green refactor of the code to change both the code and the spec, which feels like it goes against the spirit of green-to-green refactors. Separately, it also makes it harder to use the tests as complement documentation to the README and usage/summary comments.

I took a stab at refactoring two of the tests as an experiment, and came up with the following:

@test "shell version (behavior)" {
  eval "$(rbenv init -)"
  version="1.2.3"
  RBENV_SHELL=bash RBENV_VERSION="$version" run rbenv shell
  assert_success "$version"
}

@test "shell version (behavior, fish)" {
  eval "$(rbenv init -)"
  version="1.2.3"
  RBENV_SHELL=fish RBENV_VERSION="$version" run rbenv shell
  assert_success "$version"
}

This test passed:

bash-3.2$ bats test/shell.bats 
 ✓ shell integration disabled
 ✓ shell integration enabled
 ✓ no shell version
 ✓ shell version (behavior)
 ✓ shell version (behavior, fish)
 ✓ shell version
 ✓ shell version (fish)
 ✓ shell revert
 ✓ shell revert (fish)
 ✓ shell unset
 ✓ shell unset (fish)
 ✓ shell change invalid version
 ✓ shell change version
 ✓ shell change version (fish)

14 tests, 0 failures

I'm not sure if the other specs would be as easy to port over as this one was, but I'm happy to take a shot at it. First though, I wanted to check with y'all to see whether such a PR would be well-received or not, since there could be a good reason why these tests were written the way that they were.

[richiethomas]

As a follow-up to this issue, I made a PR in my forked version of the codebase which refactors some of the sh-shell specs. To recap, the intention is to ensure that the tests cover the result of the shell command (rather than, or perhaps in addition to, its implementation).

Side note

Obviously, this refactor was not something that the RBENV core team requested, and I won't be surprised or hurt if the suggestion is declined. I'm treating this mostly as a learning opportunity, one that I hope doesn't take up too much of the core team's bandwidth. I welcome any constructive criticism you might have.

Question

Most of the re-written specs pass as intended, but the shell unset spec fails. Apparently in the test, the RBENV_VERSION env var isn't getting unset in the way I'd expect.

The reason I would expect the unset op to succeed in the test is that, when I do the same thing directly in my shell, everything goes as expected:

$ export RBENV_VERSION=2.7.5
$ rbenv shell               
2.7.5
$ echo $?                   
0
$ rbenv shell --unset
$ echo $?            
0
$ echo $RBENV_VERSION 

$ 

Since the command works in the shell but not in the test, I'm guessing the test in its current form leaves out some important setup step, or is otherwise written incorrectly. As a side question, I'm also wondering if the three re-written specs which do pass are giving me a misleading result?

[mislav]

Hi, thanks for the code spike, and sorry for the late reply.

Absolutely agreed about the limitation of testing output (i.e. generated code) instead of that code's effect. I'm a big believer in integration tests being the best kind of verification for an intended feature, but in practice I find these kind of tests hard to set up and maintain over a long period of time.

One limitation of your eval-based test is that from within bats (which is implemented in bash) we can only verify the correctness of the bash implementation, but probably not zsh, fish, and others without spawning their subshells. And if we were to spawn subshells of fish, zsh, etc. for integration tests, our test suite would become dependent on the existence of those shells on the system, whereas now it's only dependent on bash. Do you think that would be a worthwhile tradeoff?

[richiethomas]

No worries, I appreciate your feedback! :-)

...if we were to spawn subshells of fish, zsh, etc. for integration tests, our test suite would become dependent on the existence of those shells on the system, whereas now it's only dependent on bash.

I'm trying to understand the implications of this. It sounds like we'd be asking RBENV contributors to install two additional shells (fish and zsh) in order to run the updated tests, is that correct? If the request is limited to installing two additional shells, that seems reasonable to me. They're each one-time, small-ish downloads- 15.5MB for fish, 15.3MB for zsh, per brew info command, and each took < 1 min to install on my machine, although admittedly one person's machine == anecdotal data.

On the other hand, if we're asking contributors to install every conceivable shell on which RBENV could be run (i.e. not just fish and zsh), that seems like a debilitatingly huge request, and we'd be better off leaving things as they are.

Another option we could consider- why not both? We could always just supplement the current testing harness with additional integration-level tests of the kind under discussion. These tests could potentially bail out early with a success code (?) if the user doesn't have the required dependencies.

One downside here is that I'd expect CI to run these supplemental tests, meaning it would need those shells as dependencies. This, in turn, means that some specs would run in CI but not locally (for users who are missing those dependencies). I don't love the idea of the test suite working differently in a local env vs. in CI, or the precedent that sets. But if these tests are nice-to-haves (instead of must-haves), maybe that's not a deal-breaker?

Thoughts? Am I over-thinking this?

[mislav]

These tests could potentially bail out early with a success code (?) if the user doesn't have the required dependencies.

I like this, as it presents a path of least disruption to contributors. We can ensure that in CI, zsh and fish are always present in addition to bash.

[richiethomas]

These tests could potentially bail out early with a success code (?) if the user doesn't have the required dependencies.

I like this, as it presents a path of least disruption to contributors. We can ensure that in CI, zsh and fish are always present in addition to bash.

Sounds good, I'll get started on a solution.

[richiethomas]

@mislav here's a first pass at a PR. I'll update it based on any subsequent feedback.