assert: Assert will only check enumerable properties

[RedYetiDev]

Version

v21.7.3

Platform

Linux <>-KALI 6.6.9-amd64 #1 SMP PREEMPT_DYNAMIC Kali 6.6.9-1kali1 (2024-01-08) x86_64 GNU/Linux

Subsystem

assert

What steps will reproduce the bug?

Setup an file like:

const assert = require("node:assert");

let objA = {};
let objB = {};

Object.defineProperty(objA, "value", { value: true, enumerable: false });
Object.defineProperty(objB, "value", { value: false, enumerable: false });

assert.deepStrictEqual(objA, objB)

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

node:assert:126
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

  {
+   value: false
-   value: true
  }
    at Object.<anonymous> (<....>)
    at Module._compile (node:internal/modules/cjs/loader:1368:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1426:10)
    at Module.load (node:internal/modules/cjs/loader:1205:32)
    at Module._load (node:internal/modules/cjs/loader:1021:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:142:12)
    at node:internal/main/run_main_module:28:49 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: { value: false },
  expected: { value: true },
  operator: 'deepStrictEqual'
}

Node.js v21.7.3

What do you see instead?

undefined

Additional information

This issue is caused because ObjectKeys (Object.keys) does not count non-enumerable objects:

node/lib/internal/util/comparisons.js

Line 274 in f425710

aKeys = ObjectKeys(val1);

View Full Function

node/lib/internal/util/comparisons.js

Lines 263 to 379 in f425710

	function keyCheck(val1, val2, strict, memos, iterationType, aKeys) {
	// For all remaining Object pairs, including Array, objects and Maps,
	// equivalence is determined by having:
	// a) The same number of owned enumerable properties
	// b) The same set of keys/indexes (although not necessarily the same order)
	// c) Equivalent values for every corresponding key/index
	// d) For Sets and Maps, equal contents
	// Note: this accounts for both named and indexed properties on Arrays.
	const isArrayLikeObject = aKeys !== undefined;
	if (aKeys === undefined) {
	aKeys = ObjectKeys(val1);
	}
	// Cheap key test
	if (aKeys.length > 0) {
	for (const key of aKeys) {
	if (!ObjectPrototypePropertyIsEnumerable(val2, key)) {
	return false;
	}
	}
	}
	if (!isArrayLikeObject) {
	// The pair must have the same number of owned properties.
	if (aKeys.length !== ObjectKeys(val2).length) {
	return false;
	}
	if (strict) {
	const symbolKeysA = ObjectGetOwnPropertySymbols(val1);
	if (symbolKeysA.length !== 0) {
	let count = 0;
	for (const key of symbolKeysA) {
	if (ObjectPrototypePropertyIsEnumerable(val1, key)) {
	if (!ObjectPrototypePropertyIsEnumerable(val2, key)) {
	return false;
	}
	ArrayPrototypePush(aKeys, key);
	count++;
	} else if (ObjectPrototypePropertyIsEnumerable(val2, key)) {
	return false;
	}
	}
	const symbolKeysB = ObjectGetOwnPropertySymbols(val2);
	if (symbolKeysA.length !== symbolKeysB.length &&
	getEnumerables(val2, symbolKeysB).length !== count) {
	return false;
	}
	} else {
	const symbolKeysB = ObjectGetOwnPropertySymbols(val2);
	if (symbolKeysB.length !== 0 &&
	getEnumerables(val2, symbolKeysB).length !== 0) {
	return false;
	}
	}
	}
	}
	if (aKeys.length === 0 &&
	(iterationType === kNoIterator ||
	(iterationType === kIsArray && val1.length === 0) ||
	val1.size === 0)) {
	return true;
	}
	// Use memos to handle cycles.
	if (memos === undefined) {
	memos = {
	set: undefined,
	a: val1,
	b: val2,
	c: undefined,
	d: undefined,
	deep: false,
	deleteFailures: false,
	};
	return objEquiv(val1, val2, strict, aKeys, memos, iterationType);
	}
	if (memos.set === undefined) {
	if (memos.deep === false) {
	if (memos.a === val1) {
	if (memos.b === val2) return true;
	}
	memos.c = val1;
	memos.d = val2;
	memos.deep = true;
	const result = objEquiv(val1, val2, strict, aKeys, memos, iterationType);
	memos.deep = false;
	return result;
	}
	memos.set = new SafeSet();
	memos.set.add(memos.a);
	memos.set.add(memos.b);
	memos.set.add(memos.c);
	memos.set.add(memos.d);
	}
	const { set } = memos;
	const originalSize = set.size;
	set.add(val1);
	set.add(val2);
	if (originalSize === set.size) {
	return true;
	}
	const areEq = objEquiv(val1, val2, strict, aKeys, memos, iterationType);
	if (areEq || memos.deleteFailures) {
	set.delete(val1);
	set.delete(val2);
	}
	return areEq;
	}

I believe that this could be resolved via the use of getOwnPropertyNames instead, but I wanted to put this as an issue before making any changes.

[climba03003]

It is actually documented as "Deep" equality means that the enumerable "own" properties of child objects are recursively evaluated also by the following rules. and it currently meet the expectation.

IMO, if it changes to match also the non-enumerable one, there should remain one function for the enumerable property only.

[MoLow]

I also think this is more a feature request than a bug. WDYT @nodejs/assert ?

[RedYetiDev]

Thanks for the information, @climba03003 and @MoLow. In any case, this feature of checking non-enumerable properties should (IMO) exist in NodeJS, even if in a separate function/flag.

If you'd like, I can work on this (unless someone else wants to take on the task?)

[benjamingr]

This was discussed a few times in the past (earliest I found without going to the v0.x repo, there are earlier requests I remember #3122 (comment) ).

It was "by design" to match user expectations, we can revisit this but I'm not sure we should.

(There are plenty of other things to fix in assert with easier consensus btw)

[github-actions]

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.

[BridgeAR]

@RedYetiDev did you have the demand for that in a real application?

I believe it is probably easy to work around this. I suggest we close this one as won't fix, depending on that input.

[RedYetiDev]

did you have the demand for that in a real application?

No. Feel free to close this.