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

String diffs not shown anymore when using node assert.equal #1614

Closed
mantoni opened this issue Mar 21, 2015 · 19 comments
Closed

String diffs not shown anymore when using node assert.equal #1614

mantoni opened this issue Mar 21, 2015 · 19 comments

Comments

@mantoni
Copy link
Contributor

mantoni commented Mar 21, 2015

I created a gist that reproduces the issue for me on OS X with node 0.10.32:
https://gist.github.com/mantoni/e2927ced1484f01b1fb8

Thanks for looking into it!

@a8m
Copy link
Contributor

a8m commented Mar 21, 2015

I'm not sure what do you mean by saying "anymore", in which version it works @mantoni ?
as far as I know, you can't get this using assert. for this reason.

@mantoni
Copy link
Contributor Author

mantoni commented Mar 21, 2015

Can't say exactly when it stopped working, but I took this screenshot once: https://www.npmjs.com/package/consolify

@mantoni
Copy link
Contributor Author

mantoni commented Mar 21, 2015

Thanks for the pointer @a8m, I removed the err.showDiff && part from the conditional in the line you mentioned and then it works perfectly fine.

@a8m
Copy link
Contributor

a8m commented Mar 21, 2015

kk, I'm on it.
trying to detect in which version it's stop to work.

update: v1.21.5
this commit: a5d7240

@a8m
Copy link
Contributor

a8m commented Mar 21, 2015

Thanks for pointing that out @mantoni.

@a8m a8m self-assigned this Mar 21, 2015
@a8m
Copy link
Contributor

a8m commented Mar 22, 2015

Is this something that we want to fix @mochajs/mocha ? if so, I'll label it as a "bug", and start to working on it.

@dasilvacontin
Copy link
Contributor

So it depends on whether the asserting library sets showDiff to true or not, right? Like, as an user, could you do something to show the diff, without modifying the library?

@mantoni
Copy link
Contributor Author

mantoni commented Mar 22, 2015

It's a feature that is advertised on the mocha page, but it only works in specific setups. So if this is not going to considered a bug, it could be a new flag to opt in and it would be nice to pit some notes on how to get it working on the web page.

@a8m
Copy link
Contributor

a8m commented Mar 22, 2015

Yes, but it's worked before (<1.21.5) @dasilvacontin . that's the reason I see it as a regression.

@dasilvacontin
Copy link
Contributor

Could you please answer my doubts/questions?

Regarding if it's a regression or not, we are making people waste time trying to figure out why the diff is no longer showing, and how to make it show it again. That's something I would only do in a major version.

We need to get this fixed this asap.

@a8m
Copy link
Contributor

a8m commented Mar 22, 2015

So it depends on whether the asserting library sets showDiff to true or not, right?

Yes.

Like, as an user, could you do something to show the diff, without modifying the library?

If you're using assert, I don't see any other way except this:

try {
  assert.equal("foo", "bar")
} catch(e) {
  e.showDiff = true
  throw e
}

@dasilvacontin
Copy link
Contributor

Thanks very much, @a8m.

Ugh, this is ugly. Maybe we could add a flag for ignoring .showDiff. Or what would you suggest?

This is not a common case, right? I wouldn't make it the default behavior to ignore .showDiff.

@a8m
Copy link
Contributor

a8m commented Mar 22, 2015

Maybe we could add a flag for ignoring ...

Flag is not a good solution for some reasons(e.g: you can't use it dynamically or for a specific assertions).

I think it should .showDiff by default and if you don't want it, you should explicitly set it to false, like this (and it will not break anything).

@dasilvacontin
Copy link
Contributor

But then the user wouldn't have a choice, it would be up to the assertion module/library, right?

@mantoni
Copy link
Contributor Author

mantoni commented Mar 24, 2015

I like what @a8m proposed because it makes Mocha work as advertised without breaking other libraries that depend on the showDiff property. The proposal brought up by @dasilvacontin make sense and I think having an option to control the behavior would certainly improve the feature. However, I don't see that being necessary to fix the issue.

@a8m
Copy link
Contributor

a8m commented Mar 25, 2015

However, I don't see that being necessary to fix the issue.

Me neither.

@dasilvacontin dasilvacontin added the status: accepting prs Mocha can use your help with this one! label Mar 25, 2015
@dasilvacontin
Copy link
Contributor

Goes. 🚢

@a8m a8m assigned a8m and unassigned a8m Mar 25, 2015
a8m added a commit to a8m/mocha that referenced this issue Mar 25, 2015
dasilvacontin added a commit that referenced this issue Mar 26, 2015
fix(reporter/base): explicitly ignore showDiff #1614
@a8m
Copy link
Contributor

a8m commented Mar 26, 2015

Thanks guys @mantoni @dasilvacontin
fixed in #1626

@a8m a8m closed this as completed Mar 26, 2015
@arve0
Copy link

arve0 commented Jul 20, 2015

For other goofer like me, remember that npm test runs local node_modules/mocha. So do a npm install -g mocha if you use mocha mytest.js.

@boneskull boneskull removed the status: accepting prs Mocha can use your help with this one! label Oct 10, 2016
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

No branches or pull requests

5 participants