[Feature] assert_variables

[fchastanet]

Hello,

Would it be possible to create a new assert function that would check that no new variable has been introduced by a function

Let's say I created this function but forgot to use local keyword for the variable element

Array::contains() {
  for element in "${@:2}"; do
    [[ "${element}" = "$1" ]] && return 0
  done
  return 1
}
declare -a a=(a b c)
Array::contains a "${a[@]}"
echo ${element}
# displays: c

after calling the method Array::contains, environment has been polluted, it could be worst as my function could have overridden an existing parent script variable.

Here my bats file with this new assert function assert_environment_side_effects

#!/usr/bin/env bash

# shellcheck source=src/batsHeaders.sh
source "$(cd "${BATS_TEST_DIRNAME}/.." && pwd)/batsHeaders.sh"

# shellcheck source=/src/Array/contains.sh
source "${BATS_TEST_DIRNAME}/contains.sh"

function Array::contains { #@test
  declare -a tab=("elem1" "elem2" "elem3")

  run Array::contains "elem0" "${tab[@]}"
  assert_failure 1
  assert_environment_side_effects
  run Array::contains "elem1" "${tab[@]}"
  assert_success
  assert_environment_side_effects
  run Array::contains "elem3" "${tab[@]}"
  assert_success
  assert_environment_side_effects
}

Here it would fail saying that variable element has been added to the environment.

Passing arguments to this function would assert that a variable has been added to the environnement if this is the expected behavior.

Maybe it could even be a default option of run function (or activable by feature flag), as in my opinion functions initializing global variables should be an edge case.

What do you think ?

[martin-schulze-vireso]

This is possible. In fact, there is a test in bats-core that does exactly this.

However, the test might be costly, especially so the value change check.

Are you aware of the shellcheck warnings about nonlocal variables?

[fchastanet]

Thanks for the quick answer I'm using shellcheck and I'm not aware about this kind of warning, I checked my case and shellcheck is not reporting any issue. I made a search on google and didn't find any reference to this I just found an opened feature request https://github.com/koalaman/shellcheck/issues/1016 maybe it is an optional feature to activate ?

I didn't find the test in bats core that would do that. I ended with this quick and dirty alternate run function it is using bash builtin function compgen

  local bats_run_diff_env_file_before="$(mktemp "${BATS_TEST_TMPDIR}/diff-env-before-XXXXXX")"
  local bats_run_diff_env_file_after="$(mktemp "${BATS_TEST_TMPDIR}/diff-env-after-XXXXXX")"
  getAllEnvVariables() {
    local regexpLocal='^(runCheckEnvLocal|lines|output|status|regexpLocal|BATS_RUN_COMMAND)$'
    compgen -v | grep -E -v "${regexpLocal}" | sort | uniq
  }
  bats_run_diff_env=""
  if [[ $keep_empty_lines ]]; then
    # 'output', 'status', 'lines' are global variables available to tests.
    # preserve trailing newlines by appending . and removing it later
    # shellcheck disable=SC2034
    output="$(
      getAllEnvVariables >"${bats_run_diff_env_file_before}"
      "$pre_command" "$@"; status=$?
      getAllEnvVariables >"${bats_run_diff_env_file_after}"
      printf .
      exit $status
    )" && status=0 || status=$?
    output="${output%.}"
    bats_run_diff_env="$(diff "${bats_run_diff_env_file_before}")"
  else
    # 'output', 'status', 'lines' are global variables available to tests.
    # shellcheck disable=SC2034
    output="$(
      getAllEnvVariables >"${bats_run_diff_env_file_before}"
      "$pre_command" "$@"; status=$?
      getAllEnvVariables >"${bats_run_diff_env_file_after}"
      exit $status
    )" && status=0 || status=$?
    # shellcheck disable=SC2034
    bats_run_diff_env="$(diff "${bats_run_diff_env_file_before}" "${bats_run_diff_env_file_after}")"
  fi

Of course this function has to refactored and has an overhead, it could be added it as optional run --checkEnv

then adding some helpers function like

• assert_env_variable_added
  • case when it is expected to create a new env variable, this function could accept a list of variables
• assert_env_variable_removed
• assert_env_variable_diff_empty
  • case when no new or removed variable expected

Do you think it could worth that I do a pull request ?

[fchastanet]

Actually I think you're right and it's shellcheck that should warn us about this I think we can close this issue Related shellcheck issues https://github.com/koalaman/shellcheck/issues/468 https://github.com/koalaman/shellcheck/issues/1395

Thanks for your work on bats