bats-core / bats-assert

Common assertions for Bats
Creative Commons Zero v1.0 Universal
94 stars 39 forks source link

test/test_helper.bash: Use bats_load_library to load bats-support #49

Closed gioele closed 1 year ago

gioele commented 1 year ago

Instead of hard-coding node_modules in the path (source of various issues: #38, #34), use the bats_load_library command to load bats-support.

bats_load_library is available since Bats v1.6.

Tested by running bats test on a Debian system with the bats-support package installed.

martin-schulze-vireso commented 1 year ago

I think we also need to set BATS_LIB_PATH.

gioele commented 1 year ago

Personally, instead of messing with BATS_LIB_PATH, I would

  1. replace the current NPM-based installation with a simple GNU-style Makefile that uses install (available on Linux, Win/WSL, BSD/macOS) and
  2. run make install at the beginning of the CI test,
  3. let the standard Bats lib path detection code do its thing.

The Makefile could be as simple as

INSTALL=install
prefix=/usr

install:
    $(INSTALL) -D load.bash $(DESTDIR)$(prefix)/lib/bats/bats-support/
    for f in src/*.bash ; do $(INSTALL) -D $$f $(DESTDIR)$(prefix)/lib/bats/bats-support/ ; done

This would streamline the installation by removing the need to use NPM, a package manager that is pretty much alien to the Bash world.

(I volunteer to implement the Makefile, either instead of or in addition to the NPM-based installation.)

martin-schulze-vireso commented 1 year ago

I am a bit hesitant to introduce yet another installation method. Currently, we have the following main paths for installations:

  1. git submodule
  2. brew/npm
  3. Linux distro package

By their nature only the third option uses a system location. The others are installed locally. In our tests, we can do as we please and should be guided by clarity and ease of use. I am not opposed to switching from npm per se, but I don't see much benefit.

gioele commented 1 year ago
1. git submodule
2. brew/npm
3. Linux distro package

By their nature only the third option uses a system location. The others are installed locally. In our tests, we can do as we please and should be guided by clarity and ease of use. I am not opposed to switching from npm per se, but I don't see much benefit.

Sure, as the Debian (co-)maintainer of these packages, I'm fine with whatever decision you take.

It's just that having a Makefile is, at least to me, sort of expected for a Bash-related project. (In fact the Debian packaging harness expects that and I had to work around the lack of Makefiles in order to package bats-*.)

martin-schulze-vireso commented 1 year ago

Pertaining to this PR is the question who is running these tests. I see the following users:

  1. the CI for this repository
  2. package maintainers who want to make sure they packaged correctly
  3. (in the future): bats-core/bats-core for an early warning system on breakages
  4. curious people?

I think regardless of the usecase, setting BATS_LIB_PATH is the correct solution.

We can debate whether it makes sense to provide a unified installation framework (bats-core has an install.sh) across the whole ecosystem to make life easier for package maintainers but I would prefer to have that discussion in an extra ticket and with input from other distro's maintainers as well.