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

test: favor deepStrictEqual over deepEqual #12883

Closed
wants to merge 1 commit into from

Conversation

@Trott
Copy link
Member

commented May 7, 2017

test-http-mutable-headers uses assert.deepEqual() in three places but
appears to only needs it in two of them. Replace one with
assert.deepStrictEqual() and remove linting exception.

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

test http

@Trott Trott added http test labels May 7, 2017

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 7, 2017

test/parallel/test-http-mutable-headers.js Outdated
@@ -79,8 +79,7 @@ const s = http.createServer(common.mustCall((req, res) => {
'set-cookie': cookies,
'x-test-array-header': arrayValues
});
// eslint-disable-next-line no-restricted-properties
assert.deepEqual(headersCopy['set-cookie'], cookies);
assert.deepStrictEqual(headersCopy['set-cookie'], cookies);

This comment has been minimized.

Copy link
@refack

refack May 7, 2017

Member

I want to suggest

      const headersCopy = res.getHeaders();
      // eslint-disable-next-line no-restricted-properties
      const expected = {
        'x-test-header': 'testing',
        'x-test-header2': 'testing',
        'set-cookie': cookies,
        'x-test-array-header': arrayValues
      };
      Object.setPrototypeOf(expected, null)
      assert.deepStrictEqual(headersCopy, expected);

And a question, why is headersCopy.__proto__ === undifined?
Also headersCopy.hasOwnPropert === undifined so the API is wrong it says the return value is an Object
https://nodejs.org/api/http.html#http_response_getheaders

This comment has been minimized.

Copy link
@addaleax

addaleax May 7, 2017

Member

And a question, why is headersCopy.__proto__ === undifined?

Because, as you noted, headersCopy has a null prototype, and therefore the __proto__ getter inherited from Object.prototype is missing.

Also headersCopy.hasOwnPropert === undifined so the API is wrong it says the return value is an Object
https://nodejs.org/api/http.html#http_response_getheaders

It’s still an object, it just doesn’t inherit from Object.prototype; if you have suggestions on how to improve the docs for this, sure.

This comment has been minimized.

Copy link
@refack

refack May 7, 2017

Member

In my books it's not an Object if headersCopy instanceof Object === false 🤷‍♂️
Ref: #12885

@addaleax
Copy link
Member

left a comment

LGTM (but if you want, feel free to go with @refack’s suggestion for changing the test code)

@refack refack dismissed a stale review May 7, 2017

No, sorry I'm make a new PR

@refack refack referenced this pull request May 7, 2017
4 of 4 tasks complete
@lpinca
lpinca approved these changes May 8, 2017
@cjihrig
cjihrig approved these changes May 8, 2017
@jasnell
jasnell approved these changes May 9, 2017
refack added a commit to refack/node that referenced this pull request May 10, 2017
doc, test: add note to response.getHeaders
* also correct language for the same note for querystring.parse
* add assertions for said note

PR-URL: nodejs#12887
Fixes: nodejs#12885
Refs: nodejs#12883
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
test: favor deepStrictEqual over deepEqual
test-http-mutable-headers uses assert.deepEqual() in three places but
appears to only needs it in two of them. Replace one with
assert.deepStrictEqual() and remove linting exception.

@Trott Trott force-pushed the Trott:strict-yo branch to 2ee8a2a May 10, 2017

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 10, 2017

@refack
refack approved these changes May 10, 2017
Trott added a commit to Trott/io.js that referenced this pull request May 11, 2017
test: favor deepStrictEqual over deepEqual
test-http-mutable-headers uses assert.deepEqual() in three places but
appears to only needs it in two of them. Replace one with
assert.deepStrictEqual() and remove linting exception.

PR-URL: nodejs#12883
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

Landed in 631cb42.

@Trott Trott closed this May 11, 2017

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
doc, test: add note to response.getHeaders
* also correct language for the same note for querystring.parse
* add assertions for said note

PR-URL: nodejs#12887
Fixes: nodejs#12885
Refs: nodejs#12883
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
test: favor deepStrictEqual over deepEqual
test-http-mutable-headers uses assert.deepEqual() in three places but
appears to only needs it in two of them. Replace one with
assert.deepStrictEqual() and remove linting exception.

PR-URL: nodejs#12883
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jun 22, 2017

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land

@Trott

This comment has been minimized.

Copy link
Member Author

commented Jun 23, 2017

@MylesBorins If #10805 gets backported, I can backport this after that lands.

@Trott Trott deleted the Trott:strict-yo branch Jul 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.