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

assert.deepEqual assertion error depth limit #15696

Closed
caub opened this issue Sep 30, 2017 · 6 comments
Closed

assert.deepEqual assertion error depth limit #15696

caub opened this issue Sep 30, 2017 · 6 comments
Labels
assert Issues and PRs related to the assert subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@caub
Copy link

caub commented Sep 30, 2017

I think it would be better if the message of assert.deepEqual was not limited to a depth of 3, and could display the first difference between the objects, like some testing libraries do

const assert = require('assert');

assert.deepEqual({
	foo: {
		bar: {
			qux: {
				lolcat: {
					bip: {
						yup: 56
					}
				}
			}
		}
	}
}, {
	foo: {
		bar: {
			qux: {
				lolcat: {
					bip: {
						yup: 56.3
					}
				}
			}
		}
	}
})

// outputs:
// AssertionError [ERR_ASSERTION]: { foo: { bar: { qux: [Object] } } } deepEqual { foo: { bar: { qux: [Object] } } }

A simple implementation example:

const deepEq = (o1, o2) => {
	const keys1 = Object.keys(o1);
	const keys2 = Object.keys(o2);
	if (keys1.length !== keys2.length) {
		assert.deepEqual(o1, o2);
	}
	for (let i=0; i<keys1.length; i++) {
		const v1 = o1[keys1[i]], v2 = o2[keys1[i]];
		if (v1 && typeof v1 === 'object') {
			deepEq(v1, v2);
		} else {
			assert.equal(v1, v2);
		}
	}
};

The current implementation passes the entire objects in the Error, and .toString is limited in 3 in depth I guess

@mscdex mscdex added the assert Issues and PRs related to the assert subsystem. label Sep 30, 2017
@tniessen tniessen added the feature request Issues that request new features to be added to Node.js. label Sep 30, 2017
@caub
Copy link
Author

caub commented Sep 30, 2017

I think there needs a different shape of AssertionError for deepEqual, maybe replacing original and expected by a string showing either:

  1. the first difference
  2. the full diff (under a max number of lines/chars)

in both case, it could be worth to represent (possibly deep) object paths maybe like:

- foo.bar.qux.lolcat.bip.yup: 56
+ foo.bar.qux.lolcat.bip.yup: 56.3

or

- foo: {bar: {qux: {lolcat: {bip: {yup: 56
+ foo: {bar: {qux: {lolcat: {bip: {yup: 56.3

What are the thoughts? I'll be glad to try to implement it

@joyeecheung
Copy link
Member

Not really object to this idea, but there is already power-assert that basically does the same thing (with very fancy diffs)

@caub
Copy link
Author

caub commented Oct 18, 2017

I checked power-assert, it's fancy, but I think a simple approach can be worth for this specific case of object compaisons (not expressions, ...)

https://repl.it/Mqyy/4

deepEqual({
	foo: {
		bar: {
			qux: {
				lolcat: {
					bip: {
						yup: 56.3
					}
				},
				mlop: {}
			}
		}
	}
}, {
	foo: {
		bar: {
			qux: {
				lolcat: {
					bip: {
						yup: 56
					}
				},
				mlep: {
					cool: 'tes',
					flip: 878.964
				}
			}
		}
	}
})
Error: ≠ [foo,bar,qux,lolcat,bip,yup]: 56.3 !== 56
       - [foo,bar,qux,mlop]: {}
       + [foo,bar,qux,mlep]: {"cool":"tes","flip"..
    at ..stacktrace... line...

@caub
Copy link
Author

caub commented Nov 18, 2017

I made this: https://github.com/caub/deep-eq.

Power-assert is cool indeed, but requires transpiling, etc..

I'd still like possibly to change node's assert deepEqual, because it's there by default, and it's really not practical

@BridgeAR
Copy link
Member

This got fixed by two PRs: #17615 (strict mode) and #17907 (non strict mode). The object might still be cut off though due to being very big.

@BridgeAR
Copy link
Member

Note: Node.js assert(value) got also improved (#17581) and is somewhat similar to the power-assert one, even though not as sophisticated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests

5 participants