assert: partialDeepStrictEqual now handles comparisons of ArrayBuffers, SharedArrayBuffers and Int16Arrays

[puskin94]

Fixes: #56097

Added handling for ArrayBuffers, SharedArrayBuffers and Int16Arrays , which were not properly considered in the new method assert.partialDeepStrictEqual .
This PR should move the feature one step closer to a stable version

[codecov]

Codecov Report

Attention: Patch coverage is 97.20670% with 5 lines in your changes missing coverage. Please review.

Project coverage is 88.01%. Comparing base (3f9c6c0) to head (b4ba259).
Report is 51 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/assert.js	97.20%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56098      +/-   ##
==========================================
+ Coverage   87.99%   88.01%   +0.01%     
==========================================
  Files         656      656              
  Lines      189000   189234     +234     
  Branches    35992    36030      +38     
==========================================
+ Hits       166308   166550     +242     
- Misses      15851    15853       +2     
+ Partials     6841     6831      -10     

Files with missing lines	Coverage Δ
lib/assert.js	99.10% <97.20%> (+0.09%)	⬆️

... and 54 files with indirect coverage changes

[aduh95]

The commit message doesn't fit our guidelines, the word just after the subsystem should be a verb in infinitive form.

[aduh95]

Let's not use instanceof, it doesn't work with values coming from other realms. Instead, let's use isArrayBuffer from internal/util/types

[aduh95]

It feels quite arbitrary to consider that comparing an ArrayBuffer and a Uint8Array would be accepted, but not any other class. I think we would benefit from separating the different cases more, and we can also avoid calling some getters.

Suggested change

	const actualView = ArrayBufferIsView(actual) ? actual : new Uint8Array(actual);
	const expectedView = ArrayBufferIsView(expected) ? expected : new Uint8Array(expected);
	if (ObjectPrototypeToString(actualView) !== ObjectPrototypeToString(expectedView)) {
	return false;
	}
	// Compare the lengths of the views (not just byte length, but actual element count)
	if (expectedView.length > actualView.length) {
	return false;
	}
	for (let i = 0; i < expectedView.length; i++) {
	let actualView, expectedView, expectedViewLength;
	if (!ArrayBufferIsView(actual)) {
	if (ArrayBufferIsView(expected)) return false;
	expectedViewLength = ArrayBufferPrototypeGetByteLength(expected)
	if (ArrayBufferPrototypeGetByteLength(actual) > expectedViewLength) return false;
	actualView = new Uint8Array(actual);
	expectedView = new Uin8Array(expected);
	} else if (isDataView(actual)) {
	if (!isDataView(expected)) return false;
	const actualByteLength = DataViewPrototypeGetByteLength(actual);
	expectedViewLength = DataViewPrototypeGetByteLength(expected)
	if (actualByteLength > expectedViewLength) return false;
	actualView = new Unit8Array(
	DataViewPrototypeGetBuffer(actual),
	DataViewPrototypeGetByteOffset(actual),
	actualByteLength,
	);
	expectedView = new Unit8Array(
	DataViewPrototypeGetBuffer(expected),
	DataViewPrototypeGetByteOffset(expected),
	expectedViewLength,
	);
	} else {
	if (ObjectPrototypeToString(actual) !== ObjectPrototypeToString(expected)) return false;
	actualView = actual;
	expectedView = expected;
	expectedViewLength = TypedArrayPrototypeGetLength(expected);
	if (TypedArrayPrototypeGetLength(actual) > expectedViewLength) return false;
	}
	for (let i = 0; i < expectedViewLength; i++) {

[aduh95]

Don't we want || here to catch all use of binary data?

Suggested change

	(ArrayBufferIsView(actual) && ArrayBufferIsView(expected)) ||
	(isArrayBuffer(actual) && isArrayBuffer(expected))
	ArrayBufferIsView(actual) || ArrayBufferIsView(expected)) ||
	isArrayBuffer(actual) || isArrayBuffer(expected)

Also, we may want to include SharedArrayBuffer (or is there a reason to treat those differently from regular ArrayBuffer?)

Suggested change

	(ArrayBufferIsView(actual) && ArrayBufferIsView(expected)) ||
	(isArrayBuffer(actual) && isArrayBuffer(expected))
	ArrayBufferIsView(actual) || ArrayBufferIsView(expected)) ||
	isAnyArrayBuffer(actual) || isAnyArrayBuffer(expected)

[aduh95]

We can optimize the happy path here

Suggested change

	ArrayBufferIsView(actual) ||
	ArrayBufferIsView(expected) ||
	isAnyArrayBuffer(actual) ||
	isAnyArrayBuffer(expected)
	ArrayBufferIsView(actual) || isAnyArrayBuffer(actual) ||
	ArrayBufferIsView(expected) || isAnyArrayBuffer(expected)

[aduh95]

Can you add a pair of [new ArrayBuffer(3), new SharedArrayBuffer(3)] and [new SharedArrayBuffer(2), new ArrayBuffer(2)] please?

[puskin94]

all the relevant tests have been added to test-assert-objects.js, including the ones you just mentioned. But I can add them here too if you want

[aduh95]

We don't need to compute the value twice

    const isActualSharedArrayBuffer = isSharedArrayBuffer(actual);

    if (isActualSharedArrayBuffer !== isSharedArrayBuffer(expected)) return false;
    
    if (isActualSharedArrayBuffer) {
      // SharedArrayBuffer is not available in primordials because it can be
      // disabled with --no-harmony-sharedarraybuffer CLI flag.
      actualViewLength = actual.byteLength;
      expectedViewLength = expected.byteLength;
    } else {
      // If it is a BufferView and not a SharedArrayBuffer, it has to be an ArrayBuffer.
      actualViewLength = ArrayBufferPrototypeGetByteLength(actual);
      expectedViewLength = ArrayBufferPrototypeGetByteLength(expected);
    }

[aduh95]

Hum actually we do want to validate that expected is indeed a BufferView

Suggested change

	if (isArrayBuffer(actual) && isArrayBuffer(expected)) {
	actualViewLength = ArrayBufferPrototypeGetByteLength(actual);
	expectedViewLength = ArrayBufferPrototypeGetByteLength(expected);
	} else if (isSharedArrayBuffer(actual) && isSharedArrayBuffer(expected)) {
	actualViewLength = actual.byteLength;
	expectedViewLength = expected.byteLength;
	} else {
	// Cannot compare ArrayBuffers with SharedArrayBuffers
	return false;
	}
	const isActualSharedArrayBuffer = isSharedArrayBuffer(actual);
	if (!isActualSharedArrayBuffer && isArrayBuffer(expected)) {
	actualViewLength = ArrayBufferPrototypeGetByteLength(actual);
	expectedViewLength = ArrayBufferPrototypeGetByteLength(expected);
	} else if (isActualSharedArrayBuffer && isSharedArrayBuffer(expected)) {
	// SharedArrayBuffer is not available in primordials because it can be
	// disabled with --no-harmony-sharedarraybuffer CLI flag.
	actualViewLength = actual.byteLength;
	expectedViewLength = expected.byteLength;
	} else {
	// Cannot compare ArrayBuffers with SharedArrayBuffers
	return false;
	}

Not sure if it actually matters for a performance PoV, feel free to ignore

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/56098
✔  Done loading data for nodejs/node/pull/56098
----------------------------------- PR info ------------------------------------
Title      assert: partialDeepStrictEqual now handles comparisons of ArrayBuffers, SharedArrayBuffers and Int16Arrays (#56098)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     puskin94:partial-deep-strict-equal-array-buffer-int-16-array -> nodejs:main
Labels     assert, author ready, needs-ci
Commits    1
 - assert: make partialDeepStrictEqual work with ArrayBuffers
Committers 1
 - Giovanni <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/56098
Fixes: https://github.com/nodejs/node/issues/56097
Reviewed-By: Antoine du Hamel <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/56098
Fixes: https://github.com/nodejs/node/issues/56097
Reviewed-By: Antoine du Hamel <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 01 Dec 2024 14:13:09 GMT
   ✔  Approvals: 1
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/56098#pullrequestreview-2486472658
   ✘  This PR needs to wait 14 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-12-07T18:10:47Z: https://ci.nodejs.org/job/node-test-pull-request/63927/
- Querying data for job/node-test-pull-request/63927/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/12216903231

[nodejs-github-bot]

Landed in dbfcbe3