assert: partialDeepStrictEqual now handles comparisons of ArrayBuffers, SharedArrayBuffers and Int16Arrays by puskin · Pull Request #56098 · nodejs/node

puskin · 2024-12-01T14:13:09Z

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 · 2024-12-01T16:10:24Z

Codecov Report

❌ 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 1829 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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

aduh95 · 2024-12-01T17:51:56Z

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

aduh95 · 2024-12-01T17:57:24Z

-    return true;
+  if (
+    (ArrayBufferIsView(actual) && ArrayBufferIsView(expected)) ||
+    (actual instanceof ArrayBuffer && expected instanceof ArrayBuffer)


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 · 2024-12-01T18:14:03Z

+  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++) {


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 · 2024-12-03T21:49:57Z

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


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 · 2024-12-04T15:41:06Z

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


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 · 2024-12-04T15:43:36Z

+      assert.throws(
+        makeBlock(assert.partialDeepStrictEqual, arrayPair[0], arrayPair[1]),
+        assert.AssertionError
+      );


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

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 · 2024-12-04T22:26:29Z

+    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;
+    }


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); }

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

Fixes: nodejs#56097

nodejs-github-bot · 2024-12-07T10:17:22Z

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

nodejs-github-bot · 2024-12-07T18:10:47Z

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

nodejs-github-bot · 2024-12-07T23:21:09Z

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 <github@puskin.it>
PR-URL: https://github.com/nodejs/node/pull/56098
Fixes: https://github.com/nodejs/node/issues/56097
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/56098
Fixes: https://github.com/nodejs/node/issues/56097
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
--------------------------------------------------------------------------------
   ℹ  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 · 2024-12-08T22:41:27Z

Landed in dbfcbe3

Fixes: #56097 PR-URL: #56098 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot added assert Issues and PRs related to the assert subsystem. needs-ci PRs that need a full CI run. labels Dec 1, 2024

puskin force-pushed the partial-deep-strict-equal-array-buffer-int-16-array branch from 248bb46 to a5f1059 Compare December 1, 2024 14:16

puskin marked this pull request as draft December 1, 2024 14:21

puskin force-pushed the partial-deep-strict-equal-array-buffer-int-16-array branch from a5f1059 to bbca465 Compare December 1, 2024 14:55

puskin marked this pull request as ready for review December 1, 2024 14:57

puskin marked this pull request as draft December 1, 2024 15:56

puskin force-pushed the partial-deep-strict-equal-array-buffer-int-16-array branch from bbca465 to 710b5cd Compare December 1, 2024 16:21

puskin marked this pull request as ready for review December 1, 2024 16:22

puskin force-pushed the partial-deep-strict-equal-array-buffer-int-16-array branch from 710b5cd to 2cc5ee6 Compare December 1, 2024 17:43

aduh95 reviewed Dec 1, 2024

View reviewed changes

Comment thread lib/assert.js

aduh95 reviewed Dec 1, 2024

View reviewed changes

puskin force-pushed the partial-deep-strict-equal-array-buffer-int-16-array branch 2 times, most recently from ca5e271 to 937f877 Compare December 2, 2024 08:21

aduh95 reviewed Dec 3, 2024

View reviewed changes

Comment thread lib/assert.js

aduh95 reviewed Dec 3, 2024

View reviewed changes

Comment thread lib/assert.js

puskin force-pushed the partial-deep-strict-equal-array-buffer-int-16-array branch 2 times, most recently from 14557cc to c912e20 Compare December 3, 2024 18:41

aduh95 reviewed Dec 3, 2024

View reviewed changes

puskin force-pushed the partial-deep-strict-equal-array-buffer-int-16-array branch from c912e20 to 2e53fae Compare December 4, 2024 13:56

aduh95 reviewed Dec 4, 2024

View reviewed changes

puskin force-pushed the partial-deep-strict-equal-array-buffer-int-16-array branch from 2e53fae to b079afc Compare December 4, 2024 16:39

aduh95 reviewed Dec 4, 2024

View reviewed changes

aduh95 approved these changes Dec 4, 2024

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 4, 2024

aduh95 reviewed Dec 6, 2024

View reviewed changes

Comment thread lib/assert.js Outdated

puskin force-pushed the partial-deep-strict-equal-array-buffer-int-16-array branch 2 times, most recently from 6ac1f1a to f94b76b Compare December 7, 2024 09:41

aduh95 reviewed Dec 7, 2024

View reviewed changes

Comment thread lib/assert.js Outdated

aduh95 reviewed Dec 7, 2024

View reviewed changes

Comment thread lib/assert.js Outdated

assert: make partialDeepStrictEqual work with ArrayBuffers

b4ba259

Fixes: nodejs#56097

puskin force-pushed the partial-deep-strict-equal-array-buffer-int-16-array branch from f94b76b to b4ba259 Compare December 7, 2024 10:01

aduh95 approved these changes Dec 7, 2024

View reviewed changes

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2024

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 7, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 7, 2024

aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 8, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 8, 2024

nodejs-github-bot merged commit dbfcbe3 into nodejs:main Dec 8, 2024

aduh95 pushed a commit that referenced this pull request Dec 10, 2024

assert: make partialDeepStrictEqual work with ArrayBuffers

cfbdff7

Fixes: #56097 PR-URL: #56098 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

github-actions Bot mentioned this pull request Dec 18, 2024

2024-12-19, Version 23.5.0 (Current) #56310

Merged

ruyadorno mentioned this pull request Dec 20, 2024

2025-01-07, Version 22.13.0 'Jod' (LTS) #56329

Merged

marco-ippolito added the dont-land-on-v20.x label Aug 18, 2025

-  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++) {

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

Uh oh!

Conversation

puskin commented Dec 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Dec 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aduh95 commented Dec 1, 2024

Uh oh!

Uh oh!

aduh95 Dec 1, 2024

Choose a reason for hiding this comment

Uh oh!

aduh95 Dec 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aduh95 Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

aduh95 Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

puskin Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

aduh95 Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 7, 2024

Uh oh!

nodejs-github-bot commented Dec 7, 2024

Uh oh!

nodejs-github-bot commented Dec 7, 2024

Uh oh!

nodejs-github-bot commented Dec 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

puskin commented Dec 1, 2024 •

edited

Loading

codecov Bot commented Dec 1, 2024 •

edited

Loading

aduh95 Dec 1, 2024 •

edited

Loading

aduh95 Dec 3, 2024 •

edited

Loading

aduh95 Dec 4, 2024 •

edited

Loading

aduh95 Dec 4, 2024 •

edited

Loading