FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

assert: Fix deepEqual/deepStrictEqual on equivalent typed arrays by feross · Pull Request #8002 · nodejs/node · GitHub

/ node Public

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

Closed
feross wants to merge 2 commits into
nodejs:masterfrom
feross:fix-8001
ConversationCommits (2)ChecksFiles changed
Closed

assert: Fix deepEqual/deepStrictEqual on equivalent typed arrays#8002
feross wants to merge 2 commits into
nodejs:masterfrom
feross:fix-8001

Conversation

feross commented Aug 6, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor
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 added the assert Issues and PRs related to the assert subsystem. label Aug 6, 2016
feross mentioned this pull request Aug 6, 2016
Closed

feross commented Aug 6, 2016

Copy link
Copy Markdown
Contributor Author

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 commented Aug 7, 2016

Copy link
Copy Markdown
Member

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 commented Aug 7, 2016

Copy link
Copy Markdown
Contributor

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 commented Aug 7, 2016

Copy link
Copy Markdown
Member

@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 mentioned this pull request Aug 7, 2016
Closed

ofrobots commented Aug 7, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

princejwesley commented Aug 7, 2016

Copy link
Copy Markdown
Contributor

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

feross added 2 commits August 7, 2016 13:51
assert: Fix deepEqual/deepStrictEqual on equivalent typed arrays
ebc9894 
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
assert: Add deepEqual/deepStrictEqual tests for typed arrays
9e7e7c3 
Let's test typed arrays which have a .byteOffset and .byteLength (i.e.
typed arrays that are slices of parent typed arrays).

feross commented Aug 7, 2016

Copy link
Copy Markdown
Contributor Author

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 commented Aug 7, 2016

Copy link
Copy Markdown
Member

@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 commented Aug 8, 2016

Copy link
Copy Markdown
Contributor

LGTM

jasnell commented Aug 8, 2016

Copy link
Copy Markdown
Member

This is a great catch @feross ... LGTM

cjihrig commented Aug 8, 2016

Copy link
Copy Markdown
Contributor

LGTM

jasnell commented Aug 8, 2016

Copy link
Copy Markdown
Member

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
assert: fix deepEqual/deepStrictEqual on equivalent typed arrays
387ab62 
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
test,assert: add deepEqual/deepStrictEqual tests for typed arrays
a8438a0 
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 commented Aug 9, 2016

Copy link
Copy Markdown
Member

Landed in 387ab62 and a8438a0

jasnell closed this Aug 9, 2016
cjihrig mentioned this pull request Aug 10, 2016
Closed
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
assert: fix deepEqual/deepStrictEqual on equivalent typed arrays
57043aa 
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
test,assert: add deepEqual/deepStrictEqual tests for typed arrays
0daceff 
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 mentioned this pull request Aug 11, 2016
Merged

MylesBorins commented Oct 10, 2016

Copy link
Copy Markdown
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 commented Oct 10, 2016

Copy link
Copy Markdown
Contributor Author

@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 modified the milestones: v4.6.2, v4.7.0 Oct 24, 2016
MylesBorins removed this from the v4.6.2 milestone Oct 26, 2016

MylesBorins commented Nov 14, 2016

Copy link
Copy Markdown
Contributor

@feross any more time lately?

feross commented Nov 23, 2016

Copy link
Copy Markdown
Contributor Author

@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
assert: fix deepEqual/deepStrictEqual on equivalent typed arrays
41546ff 
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
test,assert: add deepEqual/deepStrictEqual tests for typed arrays
f96665b 
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>
cjihrig mentioned this pull request Nov 23, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Dec 13, 2016
assert: fix deepEqual/deepStrictEqual on equivalent typed arrays
c5f82b8 
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
test,assert: add deepEqual/deepStrictEqual tests for typed arrays
c0c5608 
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
assert: fix deepEqual/deepStrictEqual on equivalent typed arrays
ee931cd 
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
test,assert: add deepEqual/deepStrictEqual tests for typed arrays
d0eb48a 
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 mentioned this pull request Dec 21, 2016
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

assert Issues and PRs related to the assert subsystem.

Projects

None yet

Milestone

4.7.0

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL