Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assert: Fix deepEqual/deepStrictEqual on equivalent typed arrays #8002

Closed
wants to merge 2 commits into from
Closed

assert: Fix deepEqual/deepStrictEqual on equivalent typed arrays #8002

wants to merge 2 commits into from

Conversation

feross
Copy link
Contributor

@feross feross commented Aug 6, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests are included
  • commit message follows commit guidelines
Affected core subsystem(s)

assert

Description of change

Fix #8001.

The typed array's underlying ArrayBuffer is used in Buffer.from.
Let's respect it's .byteOffset or .byteLength (i.e. position within the
parent ArrayBuffer).

@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Aug 6, 2016
@feross
Copy link
Contributor Author

feross commented Aug 6, 2016

For some reason, I'm getting the following test error after this change:

../test/cctest/test_inspector_socket.cc:360: Failure
Value of: err
  Actual: -16
Expected: 0
[  FAILED  ] InspectorSocketTest.WriteBeforeHandshake (1 ms)

Any ideas?

@addaleax
Copy link
Member

addaleax commented Aug 7, 2016

That test failure looks unrelated and it doesn’t happen for me, does it persist or was it just a one-off? (/cc @ofrobots @pavelfeldman @eugeneo? Maybe you would want to create a @nodejs/v8-inspector team or something for easier pinging?)

The code change itself looks good to me (thanks for noticing this!). You might want to use something like a trailing Fixes: https://github.com/nodejs/node/issues/8001 line in the commit message instead of Fix #8001 (full URLs are generally preferred for accessibility and distinction from the v0.x repo), and if you have the time, a regression test would be awesome.

CI run: https://ci.nodejs.org/job/node-test-commit/4438/

@ofrobots
Copy link
Contributor

ofrobots commented Aug 7, 2016

I can reproduce the failure on master on a mac. Seems like a regression that the CI is missing? @addaleax what platform did you try on? BTW, I have also created @nodejs/v8-inspector. Thanks for the idea.

@addaleax
Copy link
Member

addaleax commented Aug 7, 2016

@ofrobots Linux 4.4.0-31-generic #50-Ubuntu SMP Wed Jul 13 00:07:12 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux, Ubuntu 16.04. So yeah, seems totally possible that this is platform-specific.

@ofrobots
Copy link
Contributor

ofrobots commented Aug 7, 2016

Bisect points to 0190db4. I have opened a separate issue for this: #8006.
EDIT: grammar.

@princejwesley
Copy link
Contributor

I can reproduce the failure on master branch/mac platform. I tried running make cctest multiple times. Sometimes, InspectorSocketTest is running without failure.

The typed array's underlying ArrayBuffer is used in `Buffer.from`.
Let's respect it's .byteOffset or .byteLength (i.e. position within the
parent ArrayBuffer).

Fixes: #8001
Let's test typed arrays which have a .byteOffset and .byteLength (i.e.
typed arrays that are slices of parent typed arrays).
@feross
Copy link
Contributor Author

feross commented Aug 7, 2016

I fixed up the commit message and added a regression test. Thanks, @addaleax!

Since the test failures look unrelated, is this ready to merge?

@addaleax
Copy link
Member

addaleax commented Aug 7, 2016

@feross I obviously can’t speak for everyone here, but this LGTM! The commits will be squashed and the subject line shortened to something at most 50 characters in length; that can be done by you or when landing the PR, though.

@princejwesley
Copy link
Contributor

LGTM

@jasnell
Copy link
Member

jasnell commented Aug 8, 2016

This is a great catch @feross ... LGTM

@cjihrig
Copy link
Contributor

cjihrig commented Aug 8, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Aug 8, 2016

Another CI run since there was a (seemingly unrelated) failure in the last one, just to be safe: https://ci.nodejs.org/job/node-test-pull-request/3569/

jasnell pushed a commit that referenced this pull request Aug 9, 2016
The typed array's underlying ArrayBuffer is used in `Buffer.from`.
Let's respect it's .byteOffset or .byteLength (i.e. position within the
parent ArrayBuffer).

Fixes: #8001
PR-URL: #8002
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jasnell pushed a commit that referenced this pull request Aug 9, 2016
Let's test typed arrays which have a .byteOffset and .byteLength (i.e.
typed arrays that are slices of parent typed arrays).

PR-URL: #8002
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member

jasnell commented Aug 9, 2016

Landed in 387ab62 and a8438a0

@jasnell jasnell closed this Aug 9, 2016
@cjihrig cjihrig mentioned this pull request Aug 10, 2016
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
The typed array's underlying ArrayBuffer is used in `Buffer.from`.
Let's respect it's .byteOffset or .byteLength (i.e. position within the
parent ArrayBuffer).

Fixes: #8001
PR-URL: #8002
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
Let's test typed arrays which have a .byteOffset and .byteLength (i.e.
typed arrays that are slices of parent typed arrays).

PR-URL: #8002
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
@MylesBorins
Copy link
Contributor

v4.x appears to have the same problem with the example provided in #8001. Buffer.from does not exist in v4.x, but we can replicate this fix with the original buffer interface. Unfortunately there are still failures when the patch is applied

Uint8Array { '0': 2, '1': 3, '2': 4 }
Uint8Array { '0': 2, '1': 3, '2': 4 }

assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: Uint8Array { '0': 2, '1': 3, '2': 4 } deepEqual Uint8Array { '0': 2, '1': 3, '2': 4 }
    at Object.<anonymous> (/private/var/folders/ty/q7q6b07j5r3c7nvnpzm6hkq40000gn/T/test/example.js:9:8)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Function.Module.runMain (module.js:441:10)
    at startup (node.js:139:18)
    at node.js:990:3

@feross or @jasnell would you be willing to look into backporting?

@feross
Copy link
Contributor Author

feross commented Oct 10, 2016

@thealphanerd I'd love to, but I don't have the time at the moment. If this doesn't get addressed in a few weeks, feel free to ping me again and I'll do my best :)

@MylesBorins MylesBorins modified the milestones: v4.6.2, v4.7.0 Oct 24, 2016
@MylesBorins MylesBorins removed this from the v4.6.2 milestone Oct 26, 2016
@MylesBorins
Copy link
Contributor

@feross any more time lately?

@feross
Copy link
Contributor Author

feross commented Nov 23, 2016

@thealphanerd Unfortunately, no. Hopefully someone else can pick this up.

cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 23, 2016
The typed array's underlying ArrayBuffer is used in `Buffer.from`.
Let's respect it's .byteOffset or .byteLength (i.e. position within the
parent ArrayBuffer).

Fixes: nodejs#8001
PR-URL: nodejs#8002
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 23, 2016
Let's test typed arrays which have a .byteOffset and .byteLength (i.e.
typed arrays that are slices of parent typed arrays).

PR-URL: nodejs#8002
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 13, 2016
The typed array's underlying ArrayBuffer is used in `Buffer.from`.
Let's respect it's .byteOffset or .byteLength (i.e. position within the
parent ArrayBuffer).

Fixes: #8001
PR-URL: #8002
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 13, 2016
Let's test typed arrays which have a .byteOffset and .byteLength (i.e.
typed arrays that are slices of parent typed arrays).

PR-URL: #8002
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
The typed array's underlying ArrayBuffer is used in `Buffer.from`.
Let's respect it's .byteOffset or .byteLength (i.e. position within the
parent ArrayBuffer).

Fixes: #8001
PR-URL: #8002
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
Let's test typed arrays which have a .byteOffset and .byteLength (i.e.
typed arrays that are slices of parent typed arrays).

PR-URL: #8002
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants