assert: Assert will only check enumerable properties

RedYetiDev commented 4 months ago

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: https://github.com/nodejs/node/blob/f425710f13d6e1b2a9664248434f6ba6df60e861/lib/internal/util/comparisons.js#L274

View Full Function

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 commented 4 months ago

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 commented 4 months ago

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

RedYetiDev commented 4 months ago

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 commented 4 months ago

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 https://github.com/nodejs/node/issues/3122#issuecomment-149337953 ).

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)

nodejs / node