Improving Env Expansion Logic

[CauhxMilloy]

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.

[filmil]

Thank you for offering to contribute. I always get pleasantly surprised when someone seems to find this work useful. I'll need some time to read through your reasoning, and will get back to you.

[CauhxMilloy]

Totally, take your time.
I put this issue out (instead of just sending a PR) because I wanted to take the time and chat with you about what the better approach ought to be.

When I modified the expansion logic before (same person, different GH account, btw), I had specific scenarios around using $(location ...). I have even more scenarios now. Unfortunately, only the internal (Java) expansion logic handles it properly and it exposes no API to Skylark for doing it (easily) -- the exposed APIs (through ctx) aren't super great (more detail in the linked PR for Skylib).

If you wanted more proof of concept BUILD/bzl files, or if had questions and wanted to chat more, that's cool too. Let me know.

Thanks having this repo and always be so responsive when I want to add stuff in!

[CauhxMilloy]

I went ahead and cleaned up the 2 impls and pushed them to branches on my fork. Feel free to check them out and let me know how you feel.

.bzl approach: https://github.com/CauhxMilloy/bazel-bats/tree/improvedExpansionWithBzl
sh_test approach: https://github.com/CauhxMilloy/bazel-bats/tree/improvedExpansionWithShTest

(all changes are in a single commit on those respective branches)

I think the .bzl approach is better (especially with the logic added directly into this repo). But I think the sh_test approach is fine, based on preference.

[filmil]

Thanks. After taking a look, I suspect that pushing this logic upstream as much as we can is probably the best route.

Therefore I'd vote for the bzl approach, send the PR over when you have the time.

[CauhxMilloy]

Cool, put up #28. Thanks!