Spec: show diff between expected and actual values when `should eq` fails by makenowjust · Pull Request #10006 · crystal-lang/crystal

makenowjust · 2020-12-01T11:08:40Z

Fixed #9948

Screenshot:

This PR adds only diff output to the AssertionFailed message.

We can consider the following enhancements:

Make #pretty_inspect output stable on Hash keys and Set values for diff.
(by adding stable option to #pretty_inspect?)
Highlight diff output for human readability.
Add word diff by using wdiff command.

Thank you.

Fixed crystal-lang#9948

straight-shoota · 2020-12-01T12:57:00Z

src/spec/expectations.cr

+
+    begin
+      # Invoke `diff` command and fix up its output.
+      result = `#{diff_command} -u #{expected_file.path} #{actual_file.path}`


Please use Process.run/run? for better error and stream handling, path escapes etc.

straight-shoota · 2020-12-01T13:08:09Z

This looks great! I'm not sure how it behaves with long values and huge differences. Maybe the diff should be hidden when it's too long? I don't think it's particularly useful when it spans over a page or so.
I'd also be very interested in shortening the diff output. For example, are ---expected/---actual labels really necessary? The line numbers probably only make sense when there's exactly one item per line (like in a multiline string). But when you have an array of complex data structures where each can print more than one line, you the line number of the diff provides only limited information.

@oprypin It would be really helpful if you could share your concerns instead of just giving a thumbs down.

crystal-lang#10006 (comment)

makenowjust · 2020-12-01T13:58:27Z

--- expected and +++ actual lines are removed. In fact, AVA has no such lines.

src/spec/expectations.cr

oprypin · 2020-12-05T01:42:53Z

I just think that this is a maintenance liability. Running specs will now require a diff executable and with particular expectations towards how it's implemented.
GitHub Actions just happens to have the executable, but that won't always be so -- on Windows almost never.

oprypin · 2020-12-05T01:46:06Z

src/spec/expectations.cr

+    diff_command = Spec.diff_command
+    return unless diff_command && (expected.includes?('\n') || actual.includes?('\n'))
+
+    expected_file = File.tempfile("expected") { |f| f.puts expected }


If this will be merged, there should be some investigation towards limiting Crystal's tmp file footprint to a particular subdirectory. Maybe there is already a spec-global top-level directory being used somewhere? I know in Crystal's own specs there is but that's not it.

straight-shoota · 2020-12-05T01:47:56Z

@oprypin This does not require diff. It serves as an optional enhancement.

oprypin · 2020-12-05T01:52:17Z

The spec for this requires diff.

src/spec/expectations.cr

Co-authored-by: Caspian Baska <caspianbaska@gmail.com>

@oprypin

crystal-lang#10006 (comment) Thanks, @oprypin!

makenowjust · 2020-12-05T03:38:23Z

Added git diff to candidates of diff command.

~~I think we can drop the support of other diff candidates because it is rare that a Crystal developer's PC lacks git command.~~

Hmm, container env may lack git...?

jhass · 2020-12-05T09:35:04Z

We can use pending! from #9566 to mark the specs as pending when there's no diff tool available.

makenowjust · 2021-02-11T12:43:19Z

What should I do?

asterite · 2021-02-11T12:50:34Z

I think it would be nice to have a diff library in the standard library. It could start by just being text-oriented, and eventually we could make it more complex. But I think relying on the system diff might be a bit too much.

makenowjust · 2021-02-11T13:15:58Z

diff library in stdlib sounds not so bad. We have levenstein in stdlib for a specific purpose (Did you mean?). I think we can go in the same way.

oprypin · 2021-02-11T13:18:10Z

@asterite That's the opposite of what you said here #9948 (comment)
which itself was the opposite of what the discussion was tending towards at that time.

makenowjust · 2021-02-11T13:27:25Z

@asterite

It could start by just being text-oriented, and eventually we could make it more complex.

I guess and believe the latter part is impossible... (#9948 (comment))
However, the first part can implement in less than 200 LOC, so I said "not so bad".

Repeatedly, my opinion is the best idea is to use system diff because it works in 99% of environments.

asterite · 2021-02-11T13:31:49Z

@oprypin Yes, I changed my mind :-)

In any case, if others think that relying on diff is fine, then merge this.

Fixed crystal-lang#9948 Close crystal-lang#10006 This commit adds `Diff` module. It has three public methods: - `Diff.diff` computes a diff between two collections. - `Diff.unified_diff` computes a diff between two strings on lines, and returns the patch string as unified diff format. - `Diff.show_unified_diff` shows the given patch and original data as unified diff format. This implementation is less than 150 LOC 🎉 And, this commit also updates `should eq` failure message by using diff.

dsisnero · 2026-03-05T18:50:45Z

I have a diffing library that can be used. https://github.com/dsisnero/similar.cr/tree/main, ir is a port of a rust library

ysbaddaden · 2026-03-06T09:04:59Z

Nice!

I recall I added one to minitest.cr assertions, adapted from crystal-diff by @makenowjust. It's incredibly tiny.

Show diff between expected and actual values when should eq fails

d75fbb0

Fixed crystal-lang#9948

straight-shoota reviewed Dec 1, 2020

View reviewed changes

straight-shoota added kind:feature topic:stdlib:specs labels Dec 1, 2020

makenowjust added 2 commits December 1, 2020 22:48

Use Process.new("diff", ...) instead of diff ...

352168d

crystal-lang#10006 (comment)

Remove --- expected and +++ actual lines

a1423fe

caspiano reviewed Dec 5, 2020

View reviewed changes

src/spec/expectations.cr Outdated Show resolved Hide resolved

oprypin reviewed Dec 5, 2020

View reviewed changes

src/spec/expectations.cr Outdated Show resolved Hide resolved

oprypin reviewed Dec 5, 2020

View reviewed changes

src/spec/expectations.cr Outdated Show resolved Hide resolved

makenowjust and others added 2 commits December 5, 2020 11:53

Fix typo in src/spec/expectations.cr

467345b

Co-authored-by: Caspian Baska <caspianbaska@gmail.com>

Add git diff to diff command candidates

a2c7ddd

crystal-lang#10006 (comment) Thanks, @oprypin!

makenowjust added 2 commits December 7, 2020 11:34

Add Spec.use_diff? flag to determine whether it uses diff on failure

bba773a

Pending diff related specs when diff is not available

5f1a658

straight-shoota approved these changes Dec 18, 2020

View reviewed changes

straight-shoota mentioned this pull request Dec 18, 2020

Optional specs #10094

Open

straight-shoota mentioned this pull request Feb 11, 2021

[RFC] Add diff algorithm to spec output #9948

Open

Uh oh!

Conversation

makenowjust commented Dec 1, 2020

Uh oh!

straight-shoota Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

makenowjust Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

straight-shoota commented Dec 1, 2020

Uh oh!

makenowjust commented Dec 1, 2020

Uh oh!

Uh oh!

oprypin commented Dec 5, 2020

Uh oh!

oprypin Dec 5, 2020

Choose a reason for hiding this comment

Uh oh!

straight-shoota commented Dec 5, 2020

Uh oh!

oprypin commented Dec 5, 2020

Uh oh!

Uh oh!

Uh oh!

makenowjust commented Dec 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhass commented Dec 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

makenowjust commented Feb 11, 2021

Uh oh!

asterite commented Feb 11, 2021

Uh oh!

makenowjust commented Feb 11, 2021

Uh oh!

oprypin commented Feb 11, 2021

Uh oh!

makenowjust commented Feb 11, 2021

Uh oh!

asterite commented Feb 11, 2021

Uh oh!

dsisnero commented Mar 5, 2026

Uh oh!

ysbaddaden commented Mar 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

makenowjust commented Dec 5, 2020 •

edited

Loading

jhass commented Dec 5, 2020 •

edited

Loading