Skip to content
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

Revisit usage of diff with =~ in assertions #4862

Closed
josevalim opened this issue Jun 23, 2016 · 10 comments
Closed

Revisit usage of diff with =~ in assertions #4862

josevalim opened this issue Jun 23, 2016 · 10 comments

Comments

@josevalim
Copy link
Member

No description provided.

@josevalim josevalim added this to the v1.4.0 milestone Jun 23, 2016
@josevalim josevalim modified the milestone: v1.4.0 Sep 17, 2016
@josevalim josevalim added this to the v1.5.0 milestone Jan 25, 2017
@matisojka
Copy link

If possible, I'd like to help with this issue. For that I'd need to know a bit more about what needs to be done. Any inputs @josevalim or @lexmag?

@lexmag
Copy link
Member

lexmag commented May 9, 2017

@YAGoOaR this issue is about finding a way to improve difference highlighting when =~ is used, for example:

string1 = "\e[31m\n16:16:53.230 [error] GenStage consumer cannot dispatch events (an empty list must be returned): [:f, :g, :h]\n\n\e[0m"
string2 = "GenStage producer has discarded 3 events from buffer"

will give:

screen shot 2017-05-09 at 21 52 01

The output is the same as if we would use the == operator: it marks sections outside =~ matching as "deleted" (red), while they ideally should be regular color.

  1. Since =~ has left and right sides defined, we can possibly try to take a slice from left side to run difference with right on that slice. How to do that slice—that's the issue.
  2. Alternatively we should stop running difference when =~ is used.
  3. Or keep it as it is today.

:bowtie:

@josevalim
Copy link
Member Author

The problem with running the difference is that it runs it over the whole left string and that will always be misleading as we never want to compare the whole two full strings. You can see this in your snippet from the fact it is picking up letter by letter in the diff.

One possible solution is to find the longest submatch and diff only that, but that's expensive in terms of implementation and time complexity. So right now I would propose 2.

@OvermindDL1
Copy link
Contributor

I actually quite like the difference it shows. It's caught some error messages where I was expecting one thing but got another that was off by only a single character (line number specifically), and it makes it obvious in those cases.

@josevalim
Copy link
Member Author

@OvermindDL1 Are you saying that in particular to =~? If so, do you have any examples? In all errors reported because of =~ I can't recall a single situation where it actually helped. There is always too much red, as in @lexmag's example above.

@OvermindDL1
Copy link
Contributor

Sure, let me make my bug again and show the output (I'm using solarized dark as my color scheme so 'green' is more 'grayish blue'):
image
Which once the line number was fixed then it passed, without me needing to test the exception value (that is tested elsewhere as a value instead of the message).

Could always just make it an option to ExUnit too, default off or so?

@josevalim
Copy link
Member Author

josevalim commented May 10, 2017

@lexmag are we consider the string distance before computing the diff for =~?

@josevalim
Copy link
Member Author

@OvermindDL1 and thank you for the useful example! :D

@lexmag
Copy link
Member

lexmag commented May 10, 2017

@josevalim yes, my example is on the edge of acceptable threshold.
Perhaps we can have different thresholds for =~ and ==, having much lower for =~.
But it still won't solve red coloring "problem".

@josevalim
Copy link
Member Author

Closing this for now. We do not have a good answer and it seems discarding the diff means we have more to lose than to gain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants