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

Support diffs #7

Closed
wants to merge 11 commits into from
Closed

Support diffs #7

wants to merge 11 commits into from

Conversation

alexbepple
Copy link
Contributor

@rluba
Copy link
Owner

rluba commented Dec 9, 2014

Thank you for the pull request!

Generally, the diff doesn't seem very useful for containsString because the expected substring and the actual text don't necessarily have much in common:
diff-substring
In addition, the expected shown in the diff isn't really the expected value due to the semantics of containsString.

Can you provide an example where the diff is useful when using containsString? If you just want to see the actual string unescaped, I can provide an option for that and maybe even make it the default.

Regardless, the diff seems very useful for the equalTo matcher, so I've extended your changes to add support when using equalTo and/or is:
diff-equalto

Unfortunately, there's currently an open bug in mocha (including the latest 1.x and 2.x releases) that causes the diff to be displayed incorrectly: mochajs/mocha#1241

@alexbepple
Copy link
Contributor Author

When the expected substring and the actual text do have a lot in common, the diff is useful in determining the difference.

Here is a real-life example. I broke my production code by omitting the single quote after the start date. The diff makes it easier to understand the difference.

screen shot 2014-12-10 at 22 24 50

I agree, that "expected" does not match the semantics of containsString. However, it’s useful. :)

A more contrived example:

screen shot 2014-12-10 at 22 31 17

I hope you are convinced that a diff can be useful for containsString. The question now becomes whether it’s okay to make it the default. I think it is.

PS: Don’t mind my stacktraces. ;)

@rluba
Copy link
Owner

rluba commented Dec 11, 2014

Yes, the diffs in your screenshots really look useful. What version of mocha and which reporter are you using to get this diff? As you can see, my screenshot looked completely different.

@rluba
Copy link
Owner

rluba commented Dec 11, 2014

Merged into master as 0754c0f and released as part of v2.8.0.

@rluba rluba closed this Dec 11, 2014
@alexbepple
Copy link
Contributor Author

"mocha": "~2.0.1",
"mocha-unfunk-reporter": "~0.4.0"

Wow I had no idea that it was the reporter that made the diffs so useful. One more reason to stick with mocha-unfunk-reporter. ;) Originally I started using it because the default reporters were unreadable with my dark color scheme.

@rluba
Copy link
Owner

rluba commented Dec 11, 2014

Thank you for both the pull request and the pluck of mocha-unfunk-reporter :-)

@rluba
Copy link
Owner

rluba commented Dec 11, 2014

Seems like unfunk-diff is the reason for the better diffs.

@alexbepple
Copy link
Contributor Author

I am working on providing more useful diffs for IsObjectWithProperties, including only showing the parts that matter. The API will likely have to change for this, because one cannot find out what matters without having both, actual and expected. Then getExpectedForDiff() and formatActualForDiff(actual) won’t work. So perhaps this effort will result in something like getObjectsForDiff(actual) which returns a hash {actual: …, expected: …}.

In light of this, we should warn on the wiki that this is experimental.

Speaking of which, are you going to change the wiki or shall I give it a shot and then create a PR?

@rluba
Copy link
Owner

rluba commented Jan 7, 2015

It would be great if you could provide a PR after #8 is merged to document the new API. I wouldn't bother with documenting the experimental API. I've added a warning to the release notes of v2.8.0.

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

Successfully merging this pull request may close these issues.

2 participants