Currently in _bats_test_impl(), environment variables (from the env attribute) are expanded in a two step process. First, any $(location ...) (or similar) are expanded (using ctx.expand_location()), then any variables are expanded (using ctx.expand_make_variables()).
This covers many situations. However, if any variables expand to $(location ...) (or similar), an unrecoverable error is thrown (exception from Java). This level of recursive expansion is supported by built-in rules. It is unfortunate that those APIs are not exposed to Skylark at all.
For bats_tests targets to have the same env support that built-in rules have, proper expansion logic must be used. This can be done in 2 ways (this GH issue is to discuss which is preferred).
Use sh_test internally
This option is done by using sh_test as the main internal target for bats_tests. The existing custom rules (_bats_test_attrs and _bats_with_bats_assert_test_attrs) would still be generated, but the output executable is then wrapped in a sh_test. The env attribute would be passed to the sh_test -- and not to _bats_test_impl(). This allows for the environment variables to be properly expanded by the built-in (Java) implementation for built-in rules. Other environment variables which are manually set in _bats_test_impl() (e.g. TMPDIR and PATH) would remain expanding with their existing logic.
Use fancier bzl expansion logic
This option is to have better expansion logic in Skylark which mimics (and optionally improves upon) the built-in expansion logic. For more reuse, I put up a PR to Skylib (bazelbuild/bazel-skylib#486). This PR has more much detail about expansion logic in Bazel in the description. The logic in the PR also adds extra functionality beyond built-in expansion logic. This logic could either be referenced directly (adding Skylib as dep for bazel-bats) or added to a bzl file here to avoid extra deps.
I've implemented both of these approaches, and either way works fine. Let me know which you think would be better.
Currently in
_bats_test_impl(), environment variables (from theenvattribute) are expanded in a two step process. First, any$(location ...)(or similar) are expanded (usingctx.expand_location()), then any variables are expanded (usingctx.expand_make_variables()).This covers many situations. However, if any variables expand to
$(location ...)(or similar), an unrecoverable error is thrown (exception from Java). This level of recursive expansion is supported by built-in rules. It is unfortunate that those APIs are not exposed to Skylark at all.For
bats_teststargets to have the sameenvsupport that built-in rules have, proper expansion logic must be used. This can be done in 2 ways (this GH issue is to discuss which is preferred).Use
sh_testinternallyThis option is done by using
sh_testas the main internal target forbats_tests. The existing custom rules (_bats_test_attrsand_bats_with_bats_assert_test_attrs) would still be generated, but the output executable is then wrapped in ash_test. Theenvattribute would be passed to thesh_test-- and not to_bats_test_impl(). This allows for the environment variables to be properly expanded by the built-in (Java) implementation for built-in rules. Other environment variables which are manually set in_bats_test_impl()(e.g.TMPDIRandPATH) would remain expanding with their existing logic.Use fancier bzl expansion logic
This option is to have better expansion logic in Skylark which mimics (and optionally improves upon) the built-in expansion logic. For more reuse, I put up a PR to Skylib (bazelbuild/bazel-skylib#486). This PR has more much detail about expansion logic in Bazel in the description. The logic in the PR also adds extra functionality beyond built-in expansion logic. This logic could either be referenced directly (adding Skylib as dep for bazel-bats) or added to a bzl file here to avoid extra deps.
I've implemented both of these approaches, and either way works fine. Let me know which you think would be better.