search

tape-testing / tape

tap-producing test harness for node and browsers
MIT License
5.77k stars 307 forks source link

Suggestion: Use assert.deepEqual instead of deep-equal library #527

Closed dalcde closed 4 years ago

dalcde commented 4 years ago

According to the benchmarks of https://gist.github.com/substack/2790507#gistcomment-3100007 , using assert.deepEqual and util.isDeepStrictEqual is way, way faster than the deep-equal library. The results in the comment indicates deep-equal is faster only in 1 out of 9 situations, but running the script on node 14.8.0 and 10.19.0 both indicate that deep-equal is in fact slower in all situations.

Further, deep-equal has a fairly large dependency tree; in terms of package count, removing deep-equal will remove 23 out of 59 dependencies of tape.

Raynos commented 4 years ago

:+1:

ljharb commented 4 years ago

tape is meant to work in a browser, and using assert means we have to use the browserify shim, which isn't anywhere near as close to as correct as deep-equal is.

Dependency count is irrelevant, and performance is exceedingly less important than correctness. I'm strongly opposed to this change.

Raynos commented 4 years ago

This is a good point, the benefits of native assert in node 12/14 are useless when we really need to npm install assert and require('assert/')

You should open an issue on the deep-equal package to port nodejs improvements to the deep-equal package directly.

ljharb commented 4 years ago

That's already been done; deep-equal v2, which is included in tape v5, should match latest node as much as is possible.

The issue with assert/ is that it does not support old enough node/browser engines, whereas deep-equal (like tape) supports everything.

jimmywarting commented 3 years ago

I think you should diverge from doing it the NodeJS way... Use a alternative package like fast-deep-equal instead.

deep-equal is really really slow and too large! just look at it: https://npmgraph.js.org/?q=deep-equal it's a insane 2mb for something that could just be turned into a few bytes

This is the reason why i'm looking at other alternatives or simply don't use tape at all...

ljharb commented 3 years ago

I have no interest in diverging from doing it the node way, as long as node exists.

It can't be done correctly in a few bytes - fast-deep-equal is not robust. If you wish to use different alternatives, go for it, but tape (as well as the other hundreds of packages I maintain) will NEVER prioritize speed or disk space over correctness, since correctness is more important than all other considerations.