-
-
Notifications
You must be signed in to change notification settings - Fork 127
Switch to bats over pytest #598
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Note that these tests are something like 40% faster overall than the pytest suite. I'll take that :) |
| shellcheck | ||
| ; | ||
| }) | ||
| ++ (builtins.attrValues (lib.attrsets.filterAttrs (name: _val: name != "direnv-stdlib") test_pkgs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because direnv-stdlib is the result of fetchurl, it's a single-file in the store. This causes the symlinking to create the package environment to abort hard on it. We just filter it here so it is available when setting DIRENV_STDLIB but not in the package set.
fidgetingbits
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor comments from me, with the caveat that I didn't really try to understand how all the tests actually work in detail, just took a quick look and commented on what failed locally. Overall changes look good to me though. Seems much more natural to read than the pytest code was.
|
|
||
| function assert_gcroot { | ||
| profile_path=$(find "$TESTDIR/.direnv" -type l | head -n 1) | ||
| run bats_pipe find /nix/var/nix/gcroots/auto/ -type l -printf "%l\n" \| grep -q "$profile_path" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure why this fails on my system and not on yours or CI, but I get a $status == 141 error for the 4 tests that call assert_gcroot. 141 corresponds to a pipefail. This is often apparently because there is more to be written to the pipe before that isn't fully read? But not exactly sure why it'd be happening here. The default behavior of bats_pipe is to return in $status the value of the first command with a non-zero status value, and I guess 141 is coming from find piping into grep(?) so even though grep succeeds we still see the error. If I use run bats_pipe -1 (which indicates taking the error code from the Nth (zero-counted afaict) argument, then it seems to ignore the pipefail in my case and base it off greps status code. I've tested that if I purposefully cause grep to fail with a bad value, then I do get the test failure.
So I think I would suggest maybe adding -1 to bats_pipe here, with the caveat being in the unlikely event this ever changed to have additional |'s the value would have to get bumped I guess, but also happy if someone knows a better solution to this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To the best of my knowledge, grep only exits 0, 1, or 2 (and only 0 or 1 if -q is provided). Do you have that folder? What's in it? Is your nix version recent? Where does the symlink inside $TESTDIR/.direnv point? (You'll need to modify the tests to comment or remove the rm -rf $TESTDIR from util.bash's _common_teardown and probably will want to replace it with an echo of the raw $TESTDIR value).
Is this a nixos machine you're on? If no - is it a mac?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My saying to use bats_pipe -1 was meant to explain in part it's not actually grep failing. By passing -1 you're telling bats to use grep's $status value instead of any earlier failure value in $status. The command does actually work with the expected result when using bats_pipe -1.
I'm using nixos, I do have the folder and it's a regular nix gcroots folder. Nix version is:
❯ nix --version
nix (Nix) 2.31pre20250712_b1245123which is what lead to my fixing the version tests in my other PR.
❯ ls -l /tmp/nix-direnv.qQF2Rj/.direnv/
drwxr-xr-x - aa users 11 8月 22:17 bin
drwxr-xr-x - aa users 11 8月 22:17 flake-inputs
lrwxrwxrwx - aa users 11 8月 22:17 flake-profile-a5d5b61aa8a61b7d9d765e1daf971a9a578f1cfa -> /nix/store/amwjf6d2kkmb6vxnl8ia87b8384acc8s-nix-shell-env
.rw-r--r-- 57k aa users 11 8月 22:17 flake-profile-a5d5b61aa8a61b7d9d765e1daf971a9a578f1cfa.rcThis is what the profile output path was in one run, and the contents.
❯ ls -l /tmp/nix-direnv.qQF2Rj/.direnv/flake-inputs/p58d2j0ac7zvja5jl14xzbc19fakjxh2-source
.r--r--r-- 1.2k root root 1 1月 1970 allSystems.nix
.r--r--r-- 1.9k root root 1 1月 1970 check-utils.nix
.r--r--r-- 44 root root 1 1月 1970 default.nix
dr-xr-xr-x - root root 1 1月 1970 examples
.r--r--r-- 1.0k root root 1 1月 1970 filterPackages.nix
.r--r--r-- 546 root root 1 1月 1970 flake.lock
.r--r--r-- 780 root root 1 1月 1970 flake.nix
.r--r--r-- 982 root root 1 1月 1970 flattenTree.nix
.r--r--r-- 5.9k root root 1 1月 1970 lib.nix
.r--r--r-- 1.1k root root 1 1月 1970 LICENSE
.r--r--r-- 5.5k root root 1 1月 1970 README.md
.r--r--r-- 1.8k root root 1 1月 1970 simpleFlake.nixRunning the command manually works as expected:
❯ find /nix/var/nix/gcroots/auto -type l -printf "%l\n" | grep /tmp/nix-direnv.qQF2Rj/.direnv/flake-inputs/p58d2j0ac7zvja5jl14xzbc19fakjxh2-source
/tmp/nix-direnv.qQF2Rj/.direnv/flake-inputs/p58d2j0ac7zvja5jl14xzbc19fakjxh2-sourceNo errors or anything from just running find on that folder either, and $? is 0.
But if I run with bats I get the pipe error (includes the extra echo I put in for profile_path):
❯ bats -x --verbose-run ./tests/test_gc.bats
test_gc.bats
✗ use_nix_no_strict
(from function `assert_success' in file /nix/store/pgbgss9b4qr206dk3v1zl370z5v8456j-bats-assert//bats-assert/src/assert_success.bash, line 45,
from function `assert_gcroot' in file tests/test_gc.bats, line 26,
in test file tests/test_gc.bats, line 52)
`assert_gcroot' failed
$ [test_gc.bats, line 5]
$ load "util"
$ _common_setup
$$ [util.bash, line 2]
$$ shopt -s globstar
$$ bats_require_minimum_version 1.5.0
$$ bats_load_library bats-support
$$$$$$ [load.bash, line 9]
$$$$$$ source "$(dirname "${BASH_SOURCE[0]}")/src/output.bash"
$$$$$$ dirname "${BASH_SOURCE[0]}"
$$$$$$ source "$(dirname "${BASH_SOURCE[0]}")/src/error.bash"
$$$$$$ dirname "${BASH_SOURCE[0]}"
$$$$$$ source "$(dirname "${BASH_SOURCE[0]}")/src/lang.bash"
$$$$$$ dirname "${BASH_SOURCE[0]}"
$$ [util.bash, line 5]
$$ bats_load_library bats-assert
$$$$$$ [load.bash, line 22]
$$$$$$ dirname "${BASH_SOURCE[0]}"
$$$$$$ [load.bash, line 23]
$$$$$$ source "$(dirname "${BASH_SOURCE[0]}")/src/refute.bash"
$$$$$$ dirname "${BASH_SOURCE[0]}"
$$$$$$ source "$(dirname "${BASH_SOURCE[0]}")/src/assert_equal.bash"
$$$$$$ dirname "${BASH_SOURCE[0]}"
$$$$$$ source "$(dirname "${BASH_SOURCE[0]}")/src/assert_not_equal.bash"
$$$$$$ dirname "${BASH_SOURCE[0]}"
$$$$$$ source "$(dirname "${BASH_SOURCE[0]}")/src/assert_success.bash"
$$$$$$ dirname "${BASH_SOURCE[0]}"
$$$$$$ source "$(dirname "${BASH_SOURCE[0]}")/src/assert_failure.bash"
$$$$$$ dirname "${BASH_SOURCE[0]}"
$$$$$$ source "$(dirname "${BASH_SOURCE[0]}")/src/assert_output.bash"
$$$$$$ dirname "${BASH_SOURCE[0]}"
$$$$$$ source "$(dirname "${BASH_SOURCE[0]}")/src/refute_output.bash"
$$$$$$ dirname "${BASH_SOURCE[0]}"
$$$$$$ source "$(dirname "${BASH_SOURCE[0]}")/src/assert_line.bash"
$$$$$$ dirname "${BASH_SOURCE[0]}"
$$$$$$ source "$(dirname "${BASH_SOURCE[0]}")/src/refute_line.bash"
$$$$$$ dirname "${BASH_SOURCE[0]}"
$$$$$$ source "$(dirname "${BASH_SOURCE[0]}")/src/assert_regex.bash"
$$$$$$ dirname "${BASH_SOURCE[0]}"
$$$$$$ source "$(dirname "${BASH_SOURCE[0]}")/src/refute_regex.bash"
$$$$$$ dirname "${BASH_SOURCE[0]}"
$$ [util.bash, line 7]
$$ TESTDIR=
$$ TESTDIR=$(mktemp -d -t nix-direnv.XXXXXX)
$$ mktemp -d -t nix-direnv.XXXXXX
$$ export TESTDIR
$$ export DIRENV_LOG_FORMAT="direnv: %s"
$$ export NIX_USER_CONF_DIRS="$HOME/.config/nix/nix.conf:$NIX_USER_CONF_DIRS"
$$ export HOME=$TESTDIR/home
$$ unset XDG_DATA_HOME
$$ unset XDG_CONFIG_HOME
$$ cp "$BATS_TEST_DIRNAME"/testenv/* "$TESTDIR/"
$ [test_gc.bats, line 49]
$ "$@"
$ write_envrc "use nix"
$$ [util.bash, line 24]
$$ echo "source $DIRENVRC" > "$TESTDIR/.envrc"
$$ echo -e "\n$*" >> "$TESTDIR/.envrc"
$$ direnv allow "$TESTDIR"
$ [test_gc.bats, line 51]
$ assert_run_output
$$ [test_gc.bats, line 17]
$$ run_in_direnv hello
$$$ [util.bash, line 30]
$$$ run --separate-stderr direnv exec "$TESTDIR" sh -c "$@"
Hello, world!
stderr:
direnv: loading /tmp/nix-direnv.actqiO/.envrc
direnv: using nix
direnv: nix-direnv: Renewed cache
Executing shellHook.
$$$ assert_success
$$$$ [assert_success.bash, line 33]
$$$$ : "${output?}"
$$$$ : "${status?}"
$$$$ (( status != 0 ))
$$$ [util.bash, line 32]
$$$ run direnv exec "$TESTDIR" sh -c "$@"
direnv: loading /tmp/nix-direnv.actqiO/.envrc
direnv: using nix
direnv: nix-direnv: Using cached dev shell
Executing shellHook.
Hello, world!
$$$ assert_success
$$$$ [assert_success.bash, line 33]
$$$$ : "${output?}"
$$$$ : "${status?}"
$$$$ (( status != 0 ))
$$$ [util.bash, line 34]
$$$ assert_stderr -p "Renewed cache"
$$$$ [assert_output.bash, line 157]
$$$$ __assert_stream "$@"
$$$$$ [assert_output.bash, line 161]
$$$$$ local -r caller=${FUNCNAME[1]}
$$$$$ local -r stream_type=${caller/assert_/}
$$$$$ local -i is_mode_partial=0
$$$$$ local -i is_mode_regexp=0
$$$$$ local -i is_mode_nonempty=0
$$$$$ local -i use_stdin=0
$$$$$ [[ ${stream_type} == "output" ]]
$$$$$ [[ ${stream_type} == "stderr" ]]
$$$$$ : "${stderr?}"
$$$$$ local -r stream="${!stream_type}"
$$$$$ (( $# == 0 ))
$$$$$ (( $# > 0 ))
$$$$$ case "$1" in
$$$$$ is_mode_partial=1
$$$$$ shift
$$$$$ (( $# > 0 ))
$$$$$ case "$1" in
$$$$$ break
$$$$$ (( is_mode_partial ))
$$$$$ (( is_mode_regexp ))
$$$$$ local expected
$$$$$ (( use_stdin ))
$$$$$ expected="${1-}"
$$$$$ (( is_mode_nonempty ))
$$$$$ (( is_mode_regexp ))
$$$$$ (( is_mode_partial ))
$$$$$ [[ $stream != *"$expected"* ]]
$$ [test_gc.bats, line 18]
$$ assert_output -p "Hello, world"
$$$ [assert_output.bash, line 125]
$$$ __assert_stream "$@"
$$$$ [assert_output.bash, line 161]
$$$$ local -r caller=${FUNCNAME[1]}
$$$$ local -r stream_type=${caller/assert_/}
$$$$ local -i is_mode_partial=0
$$$$ local -i is_mode_regexp=0
$$$$ local -i is_mode_nonempty=0
$$$$ local -i use_stdin=0
$$$$ [[ ${stream_type} == "output" ]]
$$$$ : "${output?}"
$$$$ local -r stream="${!stream_type}"
$$$$ (( $# == 0 ))
$$$$ (( $# > 0 ))
$$$$ case "$1" in
$$$$ is_mode_partial=1
$$$$ shift
$$$$ (( $# > 0 ))
$$$$ case "$1" in
$$$$ break
$$$$ (( is_mode_partial ))
$$$$ (( is_mode_regexp ))
$$$$ local expected
$$$$ (( use_stdin ))
$$$$ expected="${1-}"
$$$$ (( is_mode_nonempty ))
$$$$ (( is_mode_regexp ))
$$$$ (( is_mode_partial ))
$$$$ [[ $stream != *"$expected"* ]]
$$ [test_gc.bats, line 19]
$$ assert_stderr -p "Executing shellHook"
$$$ [assert_output.bash, line 157]
$$$ __assert_stream "$@"
$$$$ [assert_output.bash, line 161]
$$$$ local -r caller=${FUNCNAME[1]}
$$$$ local -r stream_type=${caller/assert_/}
$$$$ local -i is_mode_partial=0
$$$$ local -i is_mode_regexp=0
$$$$ local -i is_mode_nonempty=0
$$$$ local -i use_stdin=0
$$$$ [[ ${stream_type} == "output" ]]
$$$$ [[ ${stream_type} == "stderr" ]]
$$$$ : "${stderr?}"
$$$$ local -r stream="${!stream_type}"
$$$$ (( $# == 0 ))
$$$$ (( $# > 0 ))
$$$$ case "$1" in
$$$$ is_mode_partial=1
$$$$ shift
$$$$ (( $# > 0 ))
$$$$ case "$1" in
$$$$ break
$$$$ (( is_mode_partial ))
$$$$ (( is_mode_regexp ))
$$$$ local expected
$$$$ (( use_stdin ))
$$$$ expected="${1-}"
$$$$ (( is_mode_nonempty ))
$$$$ (( is_mode_regexp ))
$$$$ (( is_mode_partial ))
$$$$ [[ $stream != *"$expected"* ]]
$ [test_gc.bats, line 52]
$ assert_gcroot
$$ [test_gc.bats, line 23]
$$ profile_path=$(find "$TESTDIR/.direnv" -type l | head -n 1)
$$ find "$TESTDIR/.direnv" -type l
$$ head -n 1
$$ echo "profile_path: $profile_path"
profile_path: /tmp/nix-direnv.actqiO/.direnv/nix-profile-25.05-086lky2c6izqghqc
$$ run bats_pipe find /nix/var/nix/gcroots/auto/ -type l -printf "%l\n" \| grep -q "$profile_path"
$$ assert_success
$$$ [assert_success.bash, line 33]
$$$ : "${output?}"
$$$ : "${status?}"
$$$ (( status != 0 ))
$$$ batslib_decorate 'command failed'
$$$ local -ir width=6
$$$ batslib_print_kv_single "$width" 'status' "$status"
$$$ fail
$$$$ [output.bash, line 130]
$$$$ local -ir col_width="$1"
$$$$ [output.bash, line 274]
$$$$ echo
$$$$ shift
$$$$ echo "-- $1 --"
$$$$ (( $# != 0 ))
$$$$ PATH="$BATS_SAVED_PATH" command cat -
$$$$ [error.bash, line 39]
$$$$ printf '%-*s : %s\n' "$col_width" "$1" "$2"
$$$$ (( $# == 0 ))
$$$$ shift 2
$$$$ batslib_err
$$$$ (( $# != 0 ))
$$$ [assert_success.bash, line 39]
$$$ batslib_print_kv_single_or_multi "$width" 'output' "$output"
$$$$$ [output.bash, line 38]
$$$$$ (( $# > 0 ))
$$$$$ PATH="$BATS_SAVED_PATH" command cat -
$$$$ [output.bash, line 183]
$$$$ local -ir width="$1"
$$$$ shift
$$$$ local -a pairs=("$@")
$$$$ local -a values=()
-- command failed --
status : 141
$$$$ local -i i
$$$$ ((i=1))
$$$$ ((i < 2))
$$$$ values+=("${pairs[$i]}")
$$$$ ((i+=2 ))
$$$$ ((i < 2))
$$$$ batslib_is_single_line "${values[@]}"
$$$$$ [output.bash, line 81]
$$$$$ for string in "$@"
$$$$$ (( $(batslib_count_lines "$string") > 1 ))
$$$$$ batslib_count_lines "$string"
$$$$$$ [output.bash, line 63]
$$$$$$ local -i n_lines=0
$$$$$$ local line
$$$$$$ IFS='' read -r line
$$$$$$ printf '%s' "$1"
$$$$$$ [[ -n $line ]]
$$$$$$ echo "$n_lines"
$$$$$ return 0
$$$$ [output.bash, line 193]
$$$$ batslib_print_kv_single "$width" "${pairs[@]}"
$$$$$ [output.bash, line 130]
$$$$$ local -ir col_width="$1"
$$$$$ shift
$$$$$ (( $# != 0 ))
$$$$$ printf '%-*s : %s\n' "$col_width" "$1" "$2"
output :
$$$$$ shift 2
$$$$$ (( $# != 0 ))
$$$ [assert_success.bash, line 40]
$$$ [[ -n "${stderr-}" ]]
$$$ batslib_print_kv_single_or_multi "$width" 'stderr' "$stderr"
$$$$ [output.bash, line 183]
$$$$ local -ir width="$1"
$$$$ shift
$$$$ local -a pairs=("$@")
$$$$ local -a values=()
$$$$ local -i i
$$$$ ((i=1))
$$$$ ((i < 2))
$$$$ values+=("${pairs[$i]}")
$$$$ ((i+=2 ))
$$$$ ((i < 2))
$$$$ batslib_is_single_line "${values[@]}"
$$$$$ [output.bash, line 81]
$$$$$ for string in "$@"
$$$$$ (( $(batslib_count_lines "$string") > 1 ))
$$$$$ batslib_count_lines "$string"
$$$$$$ [output.bash, line 63]
$$$$$$ local -i n_lines=0
$$$$$$ local line
$$$$$$ IFS='' read -r line
$$$$$$ printf '%s' "$1"
$$$$$$ (( ++n_lines ))
$$$$$$ IFS='' read -r line
$$$$$$ (( ++n_lines ))
$$$$$$ IFS='' read -r line
$$$$$$ (( ++n_lines ))
$$$$$$ IFS='' read -r line
$$$$$$ [[ -n $line ]]
$$$$$$ (( ++n_lines ))
$$$$$$ IFS='' read -r line
$$$$$$ [[ -n $line ]]
$$$$$$ echo "$n_lines"
$$$$$ return 1
$$$$ [output.bash, line 195]
$$$$ local -i i
$$$$ ((i=1))
$$$$ ((i < 2))
$$$$ pairs[$i]="$(batslib_prefix < <(printf '%s' "${pairs[$i]}"))"
$$$$ batslib_prefix < <(printf '%s' "${pairs[$i]}")
$$$$ printf '%s' "${pairs[$i]}"
$$$$$ [output.bash, line 216]
$$$$$ local -r prefix="${1:- }"
$$$$$ local line
$$$$$ IFS='' read -r line
$$$$$ printf '%s%s\n' "$prefix" "$line"
$$$$$ IFS='' read -r line
$$$$$ printf '%s%s\n' "$prefix" "$line"
$$$$$ IFS='' read -r line
$$$$$ printf '%s%s\n' "$prefix" "$line"
$$$$$ IFS='' read -r line
$$$$$ [[ -n $line ]]
$$$$$ printf '%s%s\n' "$prefix" "$line"
$$$$$ IFS='' read -r line
$$$$$ [[ -n $line ]]
$$$$ ((i+=2 ))
$$$$ ((i < 2))
$$$$ batslib_print_kv_multi "${pairs[@]}"
$$$$$ [output.bash, line 154]
$$$$$ (( $# != 0 ))
$$$$$ printf '%s (%d lines):\n' "$1" "$(batslib_count_lines "$2")"
$$$$$ batslib_count_lines "$2"
$$$$$$ [output.bash, line 63]
$$$$$$ local -i n_lines=0
$$$$$$ local line
$$$$$$ IFS='' read -r line
$$$$$$ printf '%s' "$1"
$$$$$$ (( ++n_lines ))
$$$$$$ IFS='' read -r line
$$$$$$ (( ++n_lines ))
$$$$$$ IFS='' read -r line
$$$$$$ (( ++n_lines ))
$$$$$$ IFS='' read -r line
$$$$$$ [[ -n $line ]]
$$$$$$ (( ++n_lines ))
$$$$$$ IFS='' read -r line
$$$$$$ [[ -n $line ]]
$$$$$$ echo "$n_lines"
stderr (4 lines):
$$$$$ printf '%s\n' "$2"
direnv: loading /tmp/nix-direnv.actqiO/.envrc
direnv: using nix
direnv: nix-direnv: Renewed cache
Executing shellHook.
$$$$$ shift 2
$$$$$ (( $# != 0 ))
$$$$ echo '--'
--
$$$$ echo
$$$$ [error.bash, line 40]
$$$$ return 1If I add bats_pipe -1:
❯ bats ./tests/test_gc.bats
test_gc.bats
✓ use_nix_no_strict
✓ use_nix_strict
✓ use_flake_no_strict
✓ use_flake_strict
4 tests, 0 failuresIf I use run bash -c 'find /nix/var/nix/gcroots/auto/ -type l -printf "%l\n" | grep -q "$profile_path"' instead of bats_pipe it also passes, so seems specific to using bats_pipe.
Just to confirm that it's not grep failing (as I mentioned above), if I use run bats_pipe find /nix/var/nix/gcroots/auto/ -type l -printf "%l\n" \| echo "foo" I still get the 141 error. It's not the folder itself as I tested with just a regular empty folder and find on it and it also failed. I then tested with ls instead of find and it also fails. Oddly run bats_pipe echo foo \| echo foo is okay.
These tests do fail when run via nix run too.
I don't have time to try to debug bats_pipe atm, but maybe you see something in the above that gives a hint what's going on. I should have time tomorrow to try it on a different nixos box to see if I run into the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should've been clearer. I am resistant to using bats_pipe -1 here because I don't want to write data to a closed pipe at all. That seems a poor design that we should avoid rather than sidestep with tooling, especially if neither CI nor myself can replicate. I'm not entirely sure where the problem lies at the moment, but I will make some time to figure it out soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. fwiw I've tested on three other systems and can't replicate the 141 status failures on any of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw I just took a quick through an strace log to confirm the EPIPE happens the first time find tries to write to stdout after grep process has existed. grep -q will prematurely exit on the first match, which could happen while find is still running. Why this only triggers on the one box, and not the others, I'm not exactly sure yet. But I confirm if I run without grep -q it doesn't EPIPE.
If I look at the strace on the system it works on I see find exits before grep, so never tries to write the pipe after grep has already exited.
I suppose it could be how long the first command takes on my failing system vs the other (maybe slow disks, more files, etc). My gcroots/auto folder on the failing system has ~1600 entries. The working systems all have ~200-300. Seems weird with so few files for that to be the cause though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ THIS is the investigation that makes me comfortable implementing this with bats_pipe -1. This is way beyond the depth I expected from you! I hadn't found time to do the investigation myself and I truly appreciate the dedication!
I am comfortable saying "The pipe fills and grep -q may exit early as soon as it finds a match without emptying the pipe, even before the associated find completes". Thanks. I will add the -1 and a comment explaining everything and I think we can move on.
2bb4e74 to
d83dc7f
Compare
|
I think this is ready for another round of review, if anyone finds some time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR migrates the test suite from pytest (Python) to bats (Bash Automated Testing System) with bats-assert library. This change aligns the testing framework with the project's "bash-first" philosophy and removes Python dependencies from the test infrastructure.
Key changes:
- Complete replacement of Python-based pytest tests with Bash-based bats tests
- Updated test logic for garbage collection tests to check GC root creation instead of performing actual garbage collection
- New test infrastructure using bats libraries and shell utilities
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| treefmt.nix | Removed Python linting tools (mypy, ruff) and added bats test files to shell formatting |
| tests/util.bash | New utility functions for bats test setup, teardown, and common test operations |
| tests/test_versions.bats | New bats tests for version checking functionality |
| tests/test_use_nix.py → tests/test_use_nix.bats | Converted Python tests to bats format for nix usage testing |
| tests/test_gc.py → tests/test_gc.bats | Converted Python GC tests to bats with modified logic to check GC roots instead of running collection |
| tests/nix/bats-*.nix | New Nix derivations for bats testing libraries |
| tests/default.nix | New test runner configuration using bats instead of pytest |
| Multiple Python files | Removed Python test infrastructure files |
| shell.nix, flake.nix | Updated to support bats testing environment |
| direnvrc | Fixed version parsing to handle pre-release versions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
This removes python entirely from the stack. Yet to run the tests on macOS, but they're buildable by way of nix wrapping. Worst case scenario, you cannot run them from the devshell but `nix run .#test-runner-<latest||stable>` should work okay.
|
I'm going to merge this tomorrow afternoon EDT if no one has more thoughts. Thanks! |
Depends on #593, but goes much farther.
Switches entirely to bats + bats-assert for testing.
This gets rid of pytest and allows us to be truly "bash-first".
Additionally, the logic of the gc tests is changed a bit to avoid gc'ing at all.
We know where the gc roots are created so we just check if everything has been wired up correctly. This makes test much less intrusive in non-ci scenarios.
Additionally, I believe it should run faster overall by way of doing much less work.
If you are not sure of the lifecycle:
setupin any test file is akin to a function-scoped pytest fixture pre-yield: it is run every test and acts as an initializer.teardownin any test file is akin to function-scoped pytest fixute post-yield: runs post every test and acts as a cleaner.This should be enough to understand everything here, but there's more. If you want to know, the docs are here, but feel free to ask me here; they're a bit all over the place and useful things are sometimes difficult to find.
Tagging @fidgetingbits for situational awareness and possible review.