BATS tests #462
BATS tests #462
Conversation
adamruzicka
force-pushed
the
bats
branch
4 times, most recently
from
4d3870f to
753b810
Compare
adamruzicka
force-pushed
the
bats
branch
3 times, most recently
from
171dd3b to
ee9d9f1
Compare
ofedoren
left a comment
ofedoren
left a comment
There was a problem hiding this comment.
Thanks, @adamruzicka, I guess we can squash it now.
Some really minor nits, but otherwise LGTM.
| RemoteExecutorExample.run_server | ||
| when 'client' | ||
| RemoteExecutorExample.run_client | ||
| RemoteExecutorExample.run_client(ARGV[1]&.to_i) |
There was a problem hiding this comment.
nil.to_i is 0, which makes the condition above 0.times instead of probably desired 1000.times.
There was a problem hiding this comment.
nil.to_i is indeed 0, but nil&.to_i should still be nil, right?
test/bats/helpers/common.bash
Outdated
| } | ||
|
|
||
| # Print section header for debugging | ||
| print_header() { |
There was a problem hiding this comment.
This is not being used anywhere, do we still want that?
| exec_redis() { | ||
| local cmd="$@" | ||
| podman exec "${REDIS_CONTAINER_NAME}" redis-cli ${cmd} | ||
| } |
There was a problem hiding this comment.
shellcheck is not much happy about this:
SC2124 (warning): Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.
SC2086 (info): Double quote to prevent globbing and word splitting.
Not that it's important, I'v just noticed you have a commit to make it happy.
There was a problem hiding this comment.
There was no need for a local variable anyway
| setup() { | ||
| # Setup environment variables | ||
| setup_test_env | ||
|
|
||
| # Ensure containers are running | ||
| is_postgres_running && stop_postgres | ||
| start_postgres | ||
| is_redis_running && stop_redis | ||
| start_redis | ||
| } |
There was a problem hiding this comment.
Again, not important, but inconsistent spacing: 2 spaces, whilst below it's probably a tab.
|
and squashed |
ofedoren
left a comment
ofedoren
left a comment
There was a problem hiding this comment.
Thanks, @adamruzicka, let's get this in.
2 participants
squash before merge please