tinypilot icon indicating copy to clipboard operation
tinypilot copied to clipboard

Use shellcheck for bats tests without workaround

Open jotaen4tinypilot opened this issue 1 year ago • 0 comments

Unfortunately, the shellcheck+bats situation is a bit messy right now. Once they get things in order again, we can refactor our bats bash tests:

  • Remove the workaround for the warnings about the $output/$status/$lines variables (see below)
  • Use the @test prefix notation instead of the #@test comment notation.
    • I.e., @test test_some_command() { instead of test_some_command() { #@test

Backstory / Workaround

shellcheck has built-in support for .bats files, even if these files use the (otherwise non-standard) @test prefix. However, there was a regression in shellcheck, where shellcheck now produces false positive info statements about potential modifications of the $output/$status/$lines variables, which is confusing and annoying.

Our current workaround is to put the following block at the beginning of every .bats file:

{
  # Silence shellcheck for global bats variables.
  # https://github.com/tiny-pilot/tinypilot/issues/1718
  # shellcheck disable=SC2154
  echo "${output}" "${status}" "${lines}" >/dev/null
}

This silences the SC2154 warnings that would otherwise appear.

Note that this workaround has to be wrapped in a custom “scope” (subshell), because of the shellcheck gotcha that directives immediately after the shebang apply to the entire file. The {...} ensures that the directive only ever applies to the echo statement, regardless of what else is between it and the shebang.

jotaen4tinypilot avatar Jan 12 '24 17:01 jotaen4tinypilot