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: allow circular references #6432

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@Trott
Member

Trott commented Apr 27, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

assert test

Description of change

assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

Fixes: #6416

@Trott Trott added the assert label Apr 27, 2016

@addaleax

View changes

Show outdated Hide outdated lib/assert.js
@addaleax

View changes

Show outdated Hide outdated test/parallel/test-assert.js
@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Apr 27, 2016

Member

Looks like @nodejs-github-bot wrongly labeled this, investigating at #6432

Member

Fishrock123 commented Apr 27, 2016

Looks like @nodejs-github-bot wrongly labeled this, investigating at #6432

Trott added some commits Apr 27, 2016

assert: allow circular references
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

Fixes: #6416

phillipj added a commit to nodejs/github-bot that referenced this pull request Apr 28, 2016

Fix for bad node core PR exclusive labels.
This fixes some bad labelling by ensuring that *every* file affected in
the PR actually matches an exclusive label when we're checking for exclusive labels.

Refs nodejs/node#6448 nodejs/node#6432
Closes #33
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 28, 2016

Member

Took @addaleax's suggestions, rebased, force-pushed. Hoping to hold off on CI until the pesky issues with tick processor vs. OS X are resolved.

Member

Trott commented Apr 28, 2016

Took @addaleax's suggestions, rebased, force-pushed. Hoping to hold off on CI until the pesky issues with tick processor vs. OS X are resolved.

phillipj added a commit to nodejs/github-bot that referenced this pull request Apr 28, 2016

Fix for bad node core PR exclusive labels. (#34)
This fixes some bad labelling by ensuring that *every* file affected in
the PR actually matches an exclusive label when we're checking for exclusive labels.

Refs nodejs/node#6448 nodejs/node#6432
Closes #33
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Apr 28, 2016

Member

LGTM

Member

addaleax commented Apr 28, 2016

LGTM

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 29, 2016

Member

LGTM

Member

jasnell commented Apr 29, 2016

LGTM

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Apr 29, 2016

Contributor

Isn't assert just for the core? Does the core need this change?

Contributor

thefourtheye commented Apr 29, 2016

Isn't assert just for the core? Does the core need this change?

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 29, 2016

Member

@thefourtheye asked:

Isn't assert just for the core? Does the core need this change?

It's not difficult to imagine that a test we write might want to compare two objects that contain circular references.

I think it makes sense to fix this bug before we come across that need rather than waiting for it to come up and create problems for the person who is writing that test.

This is a bug fix. If it were a new feature or an API change, the bar would be considerably higher (in my opinion, at least).

Member

Trott commented Apr 29, 2016

@thefourtheye asked:

Isn't assert just for the core? Does the core need this change?

It's not difficult to imagine that a test we write might want to compare two objects that contain circular references.

I think it makes sense to fix this bug before we come across that need rather than waiting for it to come up and create problems for the person who is writing that test.

This is a bug fix. If it were a new feature or an API change, the bar would be considerably higher (in my opinion, at least).

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 29, 2016

Member

+1 to what @Trott said.

Member

jasnell commented Apr 29, 2016

+1 to what @Trott said.

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Apr 29, 2016

Contributor

Well, most of the times we would be able to argue that features are bug fixes and vice versa.

For example, this patch could also be interpreted as, asserting circular references feature. At the same time, #2315 can be argued as, bug while asserting Maps and Sets.

Contributor

thefourtheye commented Apr 29, 2016

Well, most of the times we would be able to argue that features are bug fixes and vice versa.

For example, this patch could also be interpreted as, asserting circular references feature. At the same time, #2315 can be argued as, bug while asserting Maps and Sets.

@jasnell

This comment has been minimized.

Show comment
Hide comment

@addaleax addaleax referenced this pull request Apr 29, 2016

Closed

test: move assert to improve clarity #6428

2 of 2 tasks complete
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 29, 2016

Member

@thefourtheye I'm taking your comments to mean that you might be skeptical of the need for this, but you aren't opposed to doing this. So I'm going to land this now. (But if I'm wrong, we can revert.)

Member

Trott commented Apr 29, 2016

@thefourtheye I'm taking your comments to mean that you might be skeptical of the need for this, but you aren't opposed to doing this. So I'm going to land this now. (But if I'm wrong, we can revert.)

Trott added a commit to Trott/io.js that referenced this pull request Apr 29, 2016

assert: allow circular references
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

PR-URL: nodejs#6432
Fixes: nodejs#6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 29, 2016

Member

Landed in d3aafd0.

Member

Trott commented Apr 29, 2016

Landed in d3aafd0.

@Trott Trott closed this Apr 29, 2016

Fishrock123 added a commit that referenced this pull request May 4, 2016

assert: allow circular references
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

PR-URL: #6432
Fixes: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

joelostrowski added a commit to joelostrowski/node that referenced this pull request May 4, 2016

assert: allow circular references
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

PR-URL: nodejs#6432
Fixes: nodejs#6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

Fishrock123 added a commit that referenced this pull request May 5, 2016

2016-05-05, Version 6.1.0 (Current)
* assert: `deep{Strict}Equal()` now works correctly with circular
references. (Rich Trott) #6432
* debugger: Arrays are now formatted correctly in the debugger repl.
(cjihrig) #6448
* deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu)
#6550
* net: Introduced a `Socket#connecting` property. (Fedor Indutny)
#6404
  - Previously this information was only available as the undocumented,
internal `_connecting` property.
* process: Introduced `process.cpuUsage()`. (Patrick Mueller)
#6157
* stream: `Writable#setDefaultEncoding()` now returns `this`.
(Alexander Makarenko) #5040
* util: Two new additions to `util.inspect()`:
  - Added a `maxArrayLength` option to truncate the formatting of
Arrays. (James M Snell) #6334
    - This is set to `100` by default.
  - Added a `showProxy` option for formatting proxy intercepting
handlers. (James M Snell) #6465
    - Inspecting proxies is non-trivial and as such this is off by
default.

PR-URL: #6557

Fishrock123 added a commit that referenced this pull request May 5, 2016

2016-05-05, Version 6.1.0 (Current)
* assert: `deep{Strict}Equal()` now works correctly with circular
references. (Rich Trott) #6432
* debugger: Arrays are now formatted correctly in the debugger repl.
(cjihrig) #6448
* deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu)
#6550
  - Please see our blog post for more info on the security contents of this release:
  - https://nodejs.org/en/blog/vulnerability/openssl-may-2016/
* net: Introduced a `Socket#connecting` property. (Fedor Indutny)
#6404
  - Previously this information was only available as the undocumented,
internal `_connecting` property.
* process: Introduced `process.cpuUsage()`. (Patrick Mueller)
#6157
* stream: `Writable#setDefaultEncoding()` now returns `this`.
(Alexander Makarenko) #5040
* util: Two new additions to `util.inspect()`:
  - Added a `maxArrayLength` option to truncate the formatting of
Arrays. (James M Snell) #6334
    - This is set to `100` by default.
  - Added a `showProxy` option for formatting proxy intercepting
handlers. (James M Snell) #6465
    - Inspecting proxies is non-trivial and as such this is off by
default.

PR-URL: #6557

Fishrock123 added a commit that referenced this pull request May 5, 2016

2016-05-05, Version 6.1.0 (Current)
* assert: `deep{Strict}Equal()` now works correctly with circular
references. (Rich Trott) #6432
* debugger: Arrays are now formatted correctly in the debugger repl.
(cjihrig) #6448
* deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu)
#6550
- Please see our blog post for more info on the security contents of
this release:
- https://nodejs.org/en/blog/vulnerability/openssl-may-2016/
* net: Introduced a `Socket#connecting` property. (Fedor Indutny)
#6404
- Previously this information was only available as the undocumented,
internal `_connecting` property.
* process: Introduced `process.cpuUsage()`. (Patrick Mueller)
#6157
* stream: `Writable#setDefaultEncoding()` now returns `this`.
(Alexander Makarenko) #5040
* util: Two new additions to `util.inspect()`:
- Added a `maxArrayLength` option to truncate the formatting of
Arrays. (James M Snell) #6334
- This is set to `100` by default.
- Added a `showProxy` option for formatting proxy intercepting
handlers. (James M Snell) #6465
- Inspecting proxies is non-trivial and as such this is off by
default.

PR-URL: #6557

@twada twada referenced this pull request May 6, 2016

Closed

Allow circular references #3

azu added a commit to azu/universal-deep-strict-equal that referenced this pull request May 6, 2016

feat(assert): Allow circular references
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

fix: #3
refs:nodejs/node#6416
       nodejs/node#6432

azu added a commit to azu/universal-deep-strict-equal that referenced this pull request May 6, 2016

feat(assert): Allow circular references
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

fix: #3
refs:nodejs/node#6416
       nodejs/node#6432

@alberto alberto referenced this pull request May 7, 2016

Closed

Test failing in node 6.1 #6111

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins
Member

MylesBorins commented Jun 1, 2016

@Trott lts?

@Trott Trott added the lts-watch-v4.x label Jun 2, 2016

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Jun 2, 2016

Member

@thealphanerd Yes, if it lands cleanly.

Member

Trott commented Jun 2, 2016

@thealphanerd Yes, if it lands cleanly.

MylesBorins added a commit that referenced this pull request Jul 11, 2016

assert: allow circular references
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

PR-URL: #6432
Fixes: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins added a commit that referenced this pull request Jul 11, 2016

assert: allow circular references
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

PR-URL: #6432
Fixes: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins added a commit that referenced this pull request Jul 12, 2016

assert: allow circular references
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

PR-URL: #6432
Fixes: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins added a commit that referenced this pull request Jul 12, 2016

assert: allow circular references
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

PR-URL: #6432
Fixes: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@MylesBorins MylesBorins referenced this pull request Jul 12, 2016

Merged

v4.5.0 proposal #7688

MylesBorins added a commit that referenced this pull request Jul 14, 2016

assert: allow circular references
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

PR-URL: #6432
Fixes: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins added a commit that referenced this pull request Jul 14, 2016

assert: allow circular references
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

PR-URL: #6432
Fixes: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment