-
Notifications
You must be signed in to change notification settings - Fork 42
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
add option to show diff in assert_output and assert_equal #65
base: master
Are you sure you want to change the base?
Conversation
f7bbc92
to
77311c2
Compare
src/assert_equal.bash
Outdated
|
||
while (( $# > 0 )); do | ||
case "$1" in | ||
-d|--diff) show_diff=1; shift ;; |
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 could be a breaking change for some users that want to check if something is -d
/--diff
Unfortunately, we don't have a --
interface established here. I think at minimum we should check that there are still (at least) two parameters left after the shift.
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.
done
@@ -137,6 +141,7 @@ assert_output() { | |||
case "$1" in | |||
-p|--partial) is_mode_partial=1; shift ;; | |||
-e|--regexp) is_mode_regexp=1; shift ;; | |||
-d|--diff) show_diff=1; shift ;; |
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.
Again, --diff
might be a valid output to check for. At least, we have --
here to differentiate options from output but we should at least check that there still is a trailing output.
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.
done
Why an option? The diffing could happen automatically if |
src/assert_output.bash
Outdated
| batslib_decorate 'output differs' \ | ||
| fail | ||
if (( show_diff )); then | ||
diff <(echo "$expected") <(echo "$output") \ |
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.
The unified file format is more common than the so called normal format. It would be better to use diff -u
instead of just diff
here.
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.
good idea, done
src/assert_output.bash
Outdated
| batslib_decorate 'output differs' \ | ||
| fail | ||
else | ||
batslib_print_kv_single_or_multi 8 \ |
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.
Perhaps batslib_print_kv_single_or_multi` could grow support for diffing expected and actual?
For instance it could use wdiff
instead of diff
if it detects single-line text.
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.
Thank you for the comments. I have addressed them and also added test cases for the new functionality.
@@ -137,6 +141,7 @@ assert_output() { | |||
case "$1" in | |||
-p|--partial) is_mode_partial=1; shift ;; | |||
-e|--regexp) is_mode_regexp=1; shift ;; | |||
-d|--diff) show_diff=1; shift ;; |
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.
done
src/assert_output.bash
Outdated
| batslib_decorate 'output differs' \ | ||
| fail | ||
if (( show_diff )); then | ||
diff <(echo "$expected") <(echo "$output") \ |
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.
good idea, done
src/assert_equal.bash
Outdated
|
||
while (( $# > 0 )); do | ||
case "$1" in | ||
-d|--diff) show_diff=1; shift ;; |
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.
done
Is there an opportunity here instead of trying to bake this into the matcher, to instead have a form of Having a generic diff utility would be more composable to other existing and future matchers. And allow multiple types of formatters to coexist. (Rather than an explosion of matcher options.) This would require having a structured format/spec the assertions can construct that "renders" can consume as an api. |
I'm just giving my 2c as a happy bats user For TUI output, I find it way faster to reconcile the differences with a tool like github.com/dandavison/delta So I know what you mean about colors and the meaning of red, but it would be very useful to customize the diff command in some way. Then when it comes to comparing contents, I would welcome anything that makes it easier to write stuff like this. I am not proud but it makes tests more compact and readable. That's what you mean by matcher I suppose
|
fixes #60