Always verify mock arguments #29
Conversation
Currently, mock arguments were verified inside the mock itself. Using a mock in a script not running in errexit mode (set -e) or hiding the failure meant silent failures and successful tests. Signal the invocation failure through the mocks workspace and verify success during the verify_mocks phase. Additionally, the build script has been adjusted to avoid having to add new lines at the end of every source file. Finally, a "contains" assert has been added.
| Unexpected invocation for command 'some-command': | ||
| Got : <"\${args}"> | ||
| Expected : <"${expected}"> | ||
| OUT | ||
| exit 1 | ||
| exit 1 # Kept for historical reasons. Proper usage would be mock xyz --with-args "arg1 arg2" --and exit-code 1. |
There was a problem hiding this comment.
no really, this exit 1's goal was to break the test in case of wrong arguments. since you check that afterward now you can take that off.
There was a problem hiding this comment.
Currently, it breaks scripts that invoke it when they fail because of the exit code or when they are running in set -e mode. Removing it changes the contract with the test subject.
There was a problem hiding this comment.
Oh of course, i thought the error file lookup was after a mock but it's at the end of the test. You're right sir ;)
src/asserts.sh
Outdated
| @@ -18,3 +18,7 @@ _assert_succeeded() { | |||
| _assert_failed() { | |||
| test ${1} -ne 0 || assertion_failed "Expected failure exit code\nGot: <$1>" | |||
| } | |||
|
|
|||
| _assert_contains() { | |||
| echo "$1" | grep -q "$2" || assertion_failed "Expected: <$1>\nTo match pattern: <$2>" | |||
There was a problem hiding this comment.
should be To contain pattern cause the assert says assert contains ;)
| EXP | ||
| ) | ||
| assert "$(cat assertion_output)" equals "${expected_error}" | ||
| assert "${actual}" contains "${expected_error}" |
There was a problem hiding this comment.
asserting how problems are displayed is an important part of the user experience of this library, by using contains instead of equals you seem to be hiding something, what is it?
There was a problem hiding this comment.
The stuff being hidden is caused by the test setup, we essentially have a fake test suite and the output contains much more than the errors themselves. By using contains we can focus on the important part for that test. Otherwise, this test will break when we change, for example, the test report header or footer.
There was a problem hiding this comment.
The exact text is the following:
Running Simple Bash Tests
-------------------------
verify_arguments_order.fails_when_verifying_arguments_even_if_exit_code_is_ignored...FAILED
=========================
FAIL: verify_arguments_order.fails_when_verifying_arguments_even_if_exit_code_is_ignored
-------- STDOUT ---------
Unexpected invocation for command 'some-command':
Got : <"three trois">
Expected : <"two deux">
Unexpected invocation for command 'some-command':
Got : <"two deux">
Expected : <"three trois">
-------- STDERR ---------
-------------------------
-------------------------
Ran 1 test
>>> FAILURE (1 problem) <<<
There was a problem hiding this comment.
okay i get the extent of this now, sorry for that.
The test you're replacing is kind of part of the doc on how to use mocking... yes the tests is the doc, so it's important that it's clear and understandable.
Therefore may i ask that the test remains there and that your test becomes a new one at the end of the file ?
And i will agree to the assert contains since this only changes stuff inside the mocking mechanism.
PS. (trailing new lines :) )
| EXP | ||
| ) | ||
| assert "$(cat assertion_output)" equals "${expected_error}" | ||
| assert "${actual}" contains "${expected_error}" |
There was a problem hiding this comment.
okay i get the extent of this now, sorry for that.
The test you're replacing is kind of part of the doc on how to use mocking... yes the tests is the doc, so it's important that it's clear and understandable.
Therefore may i ask that the test remains there and that your test becomes a new one at the end of the file ?
And i will agree to the assert contains since this only changes stuff inside the mocking mechanism.
PS. (trailing new lines :) )
Currently, mock arguments were verified inside the mock itself. Using a mock in a script not running in errexit mode (set -e) or hiding the failure meant silent failures and successful tests.
Signal the invocation failure through the mocks workspace and verify success during the verify_mocks phase.
Additionally, the build script has been adjusted to avoid having to add new lines at the end of every source file.
Finally, a "contains" assert has been added.