-
Notifications
You must be signed in to change notification settings - Fork 12
Add unified diff support for luatest.assert_equals
#438
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
base: master
Are you sure you want to change the base?
Conversation
69c5c84 to
fef0177
Compare
ligurio
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.
Max, thanks for the patch!
Please take a look on my comments.
CHANGELOG.md
Outdated
| - Added support for unified diff output in `t.assert_equals()` failure messages. | ||
| The diff is shown only when the test suite is run with the `--diff` option (gh-412). |
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 functionality does no break anything, I would enable it by default and remove the option at all. The most popular case (IMHO) is using luatest via build system (CMake/Makefile) and with the option disabled by default one should need to explicitly enable it in a build system.
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.
BTW we can use unified diff only for values that can be serialized to YAML.
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 would enable it by default and remove the option at all.
Okay, done.
luatest/diff.lua
Outdated
| local function url_unescape(s) | ||
| return s:gsub("%%(%x%x)", function(h) | ||
| return string.char(tonumber(h, 16)) | ||
| end) | ||
| end |
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.
Oh, no, do not invent your own unescape function for URI.
Just use uri.unescape(), 1.
Footnotes
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.
Fixed.
test/luaunit/error_msg_test.lua
Outdated
| local actual_tbl = {a = {a = 1, b = 2, c = 3}} | ||
| local expected_tbl = {a = {a = 1, b = 5, c = 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.
These cases are trivial. Tables with userdata (use newproxy() to create a sample), cdata and custom Tarantool types (like decimal/varbinary) 1 are more interesting.
Footnotes
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 added support for custom types: decimal, datetime, uuid, and varbinary. Please check the latest changes.
| end) | ||
| end | ||
|
|
||
| local function prettify_patch(patch_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.
The function deserves a comment with high-level description a function purposes.
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.
Added.
| local ok, encoded = pcall(yaml.encode, value) | ||
| if ok then | ||
| return encoded | ||
| end |
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.
if it is not serializable to YAML then ...?
fef0177 to
1d91b35
Compare
This patch adds unified diff output for `t.assert_equals()` failures when the test suite is run with the `--diff` option, using a vendored Lua implementation of google/diff-match-patch (`luatest/vendor/diff_match_patch.lua` taken from [^1]). Closes tarantool#412 [^1]: https://github.com/google/diff-match-patch/blob/master/lua/diff_match_patch.lua
1d91b35 to
26138ab
Compare
| function M.private.set_diff_enabled(value) | ||
| diff_enabled = value and true or false | ||
| end |
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.
Sseems this function is not needed anymore, right?
As I get right, diff_enabled logic in the test was needed when option for diff support was present.
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 use this here to avoid editing old tests
luatest/test/luaunit/error_msg_test.lua
Lines 9 to 15 in 26138ab
| g.before_all(function() | |
| t.private.set_diff_enabled(false) | |
| end) | |
| g.after_all(function() | |
| t.private.set_diff_enabled(true) | |
| end) |
This patch adds unified diff output for
t.assert_equals()failures when the test suite is run with the--diffoption, using a vendored Lua implementation of google/diff-match-patch (luatest/vendor/diff_match_patch.luataken from 1).Closes #412
Here is the output for the example from the issue:
code
Footnotes
https://github.com/google/diff-match-patch/blob/master/lua/diff_match_patch.lua ↩