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

Inspect doesn't work properly for Maps and Sets - It displays an empty object #662

Closed
lucasfcosta opened this issue Apr 4, 2016 · 14 comments · Fixed by #668
Closed

Inspect doesn't work properly for Maps and Sets - It displays an empty object #662

lucasfcosta opened this issue Apr 4, 2016 · 14 comments · Fixed by #668

Comments

@lucasfcosta
Copy link
Member

As I was implementing the .deep flag support on the keys assertion I noticed that utils.inspect isn't working properly when it comes to Maps and Sets. Instead of returning the values inside the Map or Set it returns the following string: {}.

You can reproduce this behavior by doing, for example:

chai.use(function (_chai, utils) {
  var inspectSet = _utilsinspect(new Set([{iAmAn: 'item'}, {thisIs: 'anotherItem'}]));
  var inspectMap = utils.inspect(new Map([[{iAmA: 'key'}, 'aValue']]))
  console.log(inspectSet); // --> {}
  console.log(inspectMap); // --> {}
});

What do you guys think should be the desired output when inspecting maps or sets?

IMO the ideal output would be:

chai.use(function (_chai, utils) {
  var inspectSet = _utilsinspect(new Set([{iAmAn: 'item' }, {thisIs: 'anotherItem'}]));
  var inspectMap = utils.inspect(new Map([[{iAmA: 'key' }, 'aValue']]))
  console.log(inspectSet); // Desired --> [ {iAmAn: 'item' }, { thisIs: 'anotherItem' } ]
   console.log(inspectMap); // Desired --> [ [ { iAmA: 'key' }, 'aValue' ] ]
});

I think this wouldn't be a difficult fix. Maybe adding a check alongside these other checks and then calling a function to iterate through every entry and format the Map/Set members should be enough. This will be very similiar to what we already do in formatArray.

I will make sure to submit a PR for this before implementing the deep flag for Maps/Sets because this will be very useful to create good test cases.

Please let me know if you've got any idea or opinion about this 😉

@keithamus
Copy link
Member

Good work @lucasfcosta - this is definitely a valid issue. As per the roadmap issue (#457) we need to do some work to get the inspect functions from chai core into chaijs/loupe. I'd like to do this before we hit 4.0.0, and as soon as I've completed the work I'm doing with chaijs/deep-eql I'll be picking up Loupe as there's a few things I'd like to tackle with it.

@lucasfcosta
Copy link
Member Author

@keithamus Thanks for the info!
Can I tackle this one and send a PR against master here or should I start working at chaijs/loupe?

@keithamus
Copy link
Member

I've got some other bits I want to work on for chaijs/loupe so hold off on this one just for now. We'll get this fixed for 4.0 though 😄

@keithamus
Copy link
Member

Marking this as pull-request-wanted as my refactor of loupe is quite far away. If you're reading this, feel free to pick this up and make this change for loupe!

@lucasfcosta
Copy link
Member Author

Important Note

Please enable the tests added on #668 which depend on this before closing this issue.
Which, by the way, are clearly indicated on the code with comments pointing to this issue.

@maxnordlund
Copy link

maxnordlund commented Nov 26, 2018

This bit me today, and I see that 4.x has been released without loupe. So the question is, should this be done here in the meantime or is loupe ready for inclusion?

In any case, I think this should be kept open until it has been fixed, yes?

Here's node utils inspect for reference.
https://github.com/nodejs/node/blob/e958ee7a70d0af136fae8a708882cccdd4601658/lib/internal/util/inspect.js

@keithamus
Copy link
Member

@maxnordlund we're trying to track issues like this on our board: https://github.com/chaijs/chai/projects/2 as we get many duplicates or similar issues. You can see https://github.com/chaijs/chai/projects/2 for progress on this. Loupe will be released with chai 5 😄

@maxnordlund
Copy link

@maxnordlund we're trying to track issues like this on our board: https://github.com/chaijs/chai/projects/2 as we get many duplicates or similar issues. You can see https://github.com/chaijs/chai/projects/2 for progress on this.

Aha, I didn't see that.

Loupe will be released with chai 5 😄

Sweet, I'm looking forward to it. Though for native WeakMap and WeakSet, only the native inspect can look inside them, which may not be a problem but still, worth mentioning.

@keithamus
Copy link
Member

Loupe will be at-least as good as Node's console.log, but yes WeakMap and WeakSet are unobservable and so will display as something like WeakSet{...}. Promise can be observed using some node plumbing but in browsers it cannot, and as such in browsers we'll have something like Promise{...}

@jfirebaugh
Copy link
Member

This got accidentally closed because #668's description contained the text we need to fix #662 in order to enable all tests. Reopening.

@jfirebaugh jfirebaugh reopened this Jun 17, 2019
@gagern
Copy link

gagern commented Nov 14, 2019

This bug had me wondering for a while why my set was empty until I found it wasn't:

expec(new Set([1,2,3])).to.contain(7)
// => AssertionError: expected {} to include 7

I wonder whether it would make sense to leverage util.inspect when running on Node. That would not only handle sets, but also give access to any custom representation people might have implemented for their own types.

@keithamus
Copy link
Member

Here's the todo list for releasing loupe v2: chaijs/loupe#15. Once released we can bring it into Chai and probably release a 4.4 version to get this in before Chai v5.

Any one who wants to help out with the todo list, we'd super appreciate it!

@pimterry
Copy link

Just ran into this issue - it was fixed a while back, and can be closed.

Loupe is already at v2.3 in Chai, and the commented out tests described above are now uncommented and passing with useful output for Maps & Sets:

chai/test/expect.js

Lines 564 to 605 in a8359d3

if (typeof Map === 'function') {
expect(new Map).to.have.length.within(0, 0);
expect(new Map).to.have.lengthOf.within(0, 0);
var map = new Map;
map.set('a', 1);
map.set('b', 2);
map.set('c', 3);
expect(map).to.have.length.within(2, 4);
expect(map).to.have.lengthOf.within(2, 4);
err(function () {
expect(map).to.have.length.within(5, 7, 'blah');
}, "blah: expected Map{ 'a' => 1, 'b' => 2, 'c' => 3 } to have a size within 5..7");
err(function () {
expect(map).to.have.lengthOf.within(5, 7, 'blah');
}, "blah: expected Map{ 'a' => 1, 'b' => 2, 'c' => 3 } to have a size within 5..7");
}
if (typeof Set === 'function') {
expect(new Set).to.have.length.within(0, 0);
expect(new Set).to.have.lengthOf.within(0, 0);
var set = new Set;
set.add(1);
set.add(2);
set.add(3);
expect(set).to.have.length.within(2, 4);
expect(set).to.have.lengthOf.within(2, 4);
err(function () {
expect(set).to.have.length.within(5, 7, 'blah');
}, "blah: expected Set{ 1, 2, 3 } to have a size within 5..7");
err(function () {
expect(set).to.have.lengthOf.within(5, 7, 'blah');
}, "blah: expected Set{ 1, 2, 3 } to have a size within 5..7");
}
});

@keithamus
Copy link
Member

Awesome thanks for the update @pimterry

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

Successfully merging a pull request may close this issue.

6 participants