lib: generalize handling in assert.deepEqual()

[Trott]

Error objects have non-enumerable properties. So, unexpectedly,
assert.deepEqual(new Error('a'), new Error('b')); will not throw. This
commit changes that behavior.

Fixes: #3122

[thefourtheye]

Whenever there is a change suggested in this module I judge that by this comment #2315 (comment). If we go by that, this change is not necessary as assert is not a general purpose module.

[ChALkeR]

Is this a semver-major? @rvagg

[Trott]

@thefourtheye Yes, if that's the standard, then this is probably not necessary. I say "probably" because I wonder if any of the checks on things like err.message in Node's own tests would be preferable to do with something like assert.deepEqual() if it worked on tests. Quite possibly not. I might start looking though.

More importantly, perhaps: If the assert module is not a general purpose module, that fact should be reflected in the docs.

[ChALkeR]

@Trott, @domenic It's a bit too late for making the assert module interal, as it looks to me. A doc-only deprecation would do no harm, but it's going to stay in that state forever.

It's directly used by at least 11% of modules on npmjs (a quick grep for require\(.assert\.) found it in 20734 packages out of 186231, and I'm sure there are false negatives). Mostly in tests, benchmarks, and specs, but still.

Update: Without the .*/(tests?|bench|benchmarks?|spec|examples?)/.* paths — 6311 packages (3%).

[rvagg]

Yes, has to be semver-major I reckon, also I'm not sure this approach is consistent, have a look at how RegExp is handled, comparing only a strict list of properties but this change will compare any properties attached to an Error—attaching custom properties to an Error is fairly common too (although not so for RegExp so perhaps there's an argument to be made there, but the inconsistency concerns me more).

[Fishrock123]

JavaScript is an awful language sometimes haha. I agree with Rod's sentiment, but I think the frequency of use would warrant this enough.

If nothing else, we could probably get it into deepStrictEquals. :)

[Trott]

I'm thinking I should close this PR and instead update the docs to reflect that:

• this assertion library is intended to only meet the needs of the Node.js project and people are encouraged to use userland assertion libraries instead (there's certainly enough of them!)
• comparison is only with enumerable properties
• that can cause unexpected results like this one
• show a workaround for it if there is a reasonably compact example workaround (such as maybe just using assert.throws() with the error type checking and message checking options)

[Trott]

@rvagg, @Fishrock123: Since it's already semver-major, might an appropriate course of action be to run RegExp (and maybe a few other things) through this more rigorous checking? Assuming it doesn't bork any of our tests, it seems like that might streamline the code a bit. It's semver-major already, so why not take the opportunity to streamline the code a bit?

[Trott]

Just pushed something to remove special handling for Error, Buffer, and RegExp. Added a tiny bit of hopefully-straightforward special handling for Array and String (which is to ignore the length property if they are being compared against something that doesn't have a length property). All tests still pass. Still semver major, but I feel much better about this. Seems less hacky. PTAL @rvagg @Fishrock123

Still could use a doc update to clarify some finer points, but that could be a separate PR, as this is all consistent with existing docs.

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/477/

[rvagg]

yeah, I don't know, this is pretty significant, perhaps we need to include @nodejs/tsc on this for more opinions

[trevnorris]

This PR looks like it's attempting to achieve some sort of theoretical completeness. Which I don't disagree with, but on this route you must seriously consider also doing proper comparisons of any Symbols also attached to the object.

[Trott]

@trevnorris Symbols added. Thanks for that!

[domenic]

I'm thinking I should close this PR and instead update the docs to reflect that:

This seems better to me.

@Trott, @domenic It's a bit too late for making the assert module interal, as it looks to me. A doc-only deprecation would do no harm, but it's going to stay in that state forever.

Nobody suggested making it internal or deprecating it. Just, not changing it.

[Trott]

@domenic Can you elaborate a bit on why you favor the doc-only approach but not a deprecation? I'm assuming deprecation could just mean sticking a deprecation notice in the docs but not doing anything at all in the code. Maybe we just have different ideas about what "deprecation" might entail.

The close-in-favor-of-doc-PR was how I felt when this change was adding additional special handling for Error objects. It's evolved, though, to where it's removing special handling for things like RegExp and Buffer and still coming up with the right answer. At the same time, it's fixing at least one thing I would consider a bug (it's now handling Error in a way that conforms with naive user expectations) and does not break any of our tests. It probably wouldn't break many tests in the wild (although it might surface tests that are passing that actually shouldn't!).

[Trott]

New CI: https://ci.nodejs.org/job/node-test-pull-request/485/

[domenic]

I just think that contributors and core team members should not spend time working on the assert module. They especially shouldn't be doing so in a way that changes behavior in observable ways that could break programs using it.

Maybe it would be helpful to add something to the docs saying its behavior won't be changed, so that people don't file issues on it. And maybe it'd even be interesting to explain why---because the assert module is developed so that Node.js can test itself, not so that you can write your own tests. I don't know if you'd call that a deprecation or not.

[Trott]

@domenic Thanks, that's very clear. I guess regardless of what happens to this PR, the doc change is a good idea.

[Trott]

Proposed doc-only update at #3330. I'm still in favor of the changes here, but regardless, the docs should reflect the current status.

[Trott]

Based on the audio from the most recent TSC meeting, I'm going to close this PR. Assert will be set to Stability Index Locked, users can generally be gently encouraged to use userland assertion libraries, and anything Node core might need added to Assert should probably go in test/common.js instead.