Make duplicate test names a hard error #286
Conversation
Previous behaviour was to issue a warning. This now exits with status code 1.
| @@ -43,7 +49,7 @@ if (type -p parallel &>/dev/null); then | |||
| # shellcheck disable=SC2034 | |||
| have_gnu_parallel=1 | |||
| elif [[ "$num_jobs" != 1 ]]; then | |||
| printf 'bats: cannot execute "%s" jobs without GNU parallel\n' "$num_jobs" >&2 | |||
| abort "Cannot execute \"${num_jobs}\" jobs without GNU parallel" | |||
| exit 1 | |||
| @@ -78,7 +83,7 @@ for filename in "$@"; do | |||
| done < <(BATS_TEST_FILTER="$filter" bats-preprocess "$filename") | |||
|
|
|||
| if [[ "${#test_dupes[@]}" -ne 0 ]]; then | |||
| printf 'bats warning: duplicate test name(s) in %s: %s\n' "$filename" "${test_dupes[*]}" >&2 | |||
| abort "Duplicate test name(s) in file \"${filename}\": ${test_dupes[*]}" | |||
There was a problem hiding this comment.
This will immediately stop execution, right?
Did you consider only failing in the end including the failure message as I suggested in the issue?
There was a problem hiding this comment.
I agree with @Vampire. The full test run is still valuable feedback. Marking just the duplicated test as failed is a useful alternative. Though we probably want to distinguish that failure somehow that it's different from a normal test failure.
There was a problem hiding this comment.
I did — my reasoning was that it's less confusing to error immediately and fail fast, otherwise if all tests pass there's a passing suite but a failing exit code and possibly a missed error.
There was a problem hiding this comment.
That's why I said the error should also be printed last.
I agree that printing the error above all tests and then having a negative exit code is too quiet, especially as during manual invocation you will easily miss that there was a non-0 exit code.
Either @jasonkarns suggestion to actually fail the tests in question would be a good idea, or printing the errors in the end accompanied with the non-0 exit code, like I did here: svn-all-fast-export/svn2git@ece0088
There was a problem hiding this comment.
It feels non-obvious to fail after passing test suite, for a check done in pre-processing. Users should rename the test anyway.
I wonder if there's also a risk of nondeterminism here with parallel --jobs, the "second" duplicate suite may sometimes run as the first. This wouldn't break anything (we've already got a breaking change for the build failing) but if they give different results is a misdirecting user experience. Needs a test.
It seems faster, simpler, and more obvious to fail when the duplicate is detected. That said we can discuss on a PR if preferred 🙏
There was a problem hiding this comment.
No, it's fine, if it is a reasoned decision rather than a read over of my comment, it's fine.
Especially as it fails at the very beginning.
There was a problem hiding this comment.
I'm not too bothered by the early failure, because it denotes a mistake in the test setup itself (as sublimino said), but if we really wanted to be more graceful I think a better fix would be to not key off (only) the user-provided test description, and instead ensure that the generated function name is unique.
There was a problem hiding this comment.
Which would still make it hard to connect failure to test then, I like it failing for duplicate test names. :-)
While then there could be a configuration option on whether to fail or not if there are duplicate names, while either way at least the tests would run correctly.
Fixes #278.