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.

Sign up for GitHub

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

Jump to bottom

test: add Symbol test for assert.deepEqual() #3327

Closed
wants to merge 1 commit into from
Closed

test: add Symbol test for assert.deepEqual() #3327

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 12, 2015

The existing test never ran because typeof Symbol === 'function' and not
'symbol'. Update to Symbol() which will be a symbol and not a function.

@Trott
test: add Symbol test for assert.deepEqual()
dd09da8 
The existing test never ran because typeof Symbol === 'function' and not
'symbol'. Update to Symbol() which will be a symbol and not a function.
@Trott Trott added the test Issues and PRs related to the tests. label Oct 12, 2015
@Trott
Copy link
Member Author

Trott commented Oct 12, 2015

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

@evanlucas
Copy link
Contributor

I would think we should maybe even go as far as adding tests for things like:

assert.throws(function() {
  assert.deepEqual(Symbol('a'), Symbol('a'));
});
assert.deepEqual(Symbol.for('a'), Symbol.for('a'));

What are your thoughts on that?

@mscdex mscdex added the assert Issues and PRs related to the assert subsystem. label Oct 12, 2015
@Trott
Copy link
Member Author

Trott commented Oct 12, 2015

@evanlucas I think your first example is handled in this test addition over at #3124. However, that PR very possibly may never land, so let's put a pin in that for sure....

The second example you give, oh yes!

@vkurchatkin
Copy link
Contributor

It would be nice to have code coverage to catch such things

@jasnell
Copy link
Member

jasnell commented Oct 14, 2015

LGTM

@Trott
Copy link
Member Author

Trott commented Oct 16, 2015

@evanlucas On the one hand, #3124 got closed, so that suggests adding the tests you proposed separately. On the other hand, TSC agreed this week to lock Assert so any tests added would presumably only document existing behavior. So... ¯\_(ツ)_/¯

Trott added a commit that referenced this pull request Oct 16, 2015
@Trott
test: add Symbol test for assert.deepEqual()
0f99320 
The existing test never ran because typeof Symbol === 'function' and not
'symbol'. We have Symbols now so remove the check and just run the test.

PR-URL: #3327
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member Author

Trott commented Oct 16, 2015

Landed in 0f99320

@Trott Trott closed this Oct 16, 2015
@MylesBorins
Copy link
Contributor

If we are going to be backporting tests for LTS this should probably be tagged

@Trott
Copy link
Member Author

Trott commented Oct 16, 2015

Would the right tag be land-on-v4.x?

@MylesBorins
Copy link
Contributor

I believe so /cc @jasnell

@MylesBorins
Copy link
Contributor

Got an update.

lts-watch-v4x if the decision hasn't been made yet

@Trott
Copy link
Member Author

Trott commented Oct 16, 2015

OK, tagged. Leave it closed, reopen it, or doesn't matter either way?

@jasnell
Copy link
Member

jasnell commented Oct 16, 2015

Doesn't matter either way (open or closed)

On Fri, Oct 16, 2015 at 2:45 PM, Rich Trott [email protected]
wrote:

OK, tagged. Leave it closed, reopen it, or doesn't matter either way?

—
Reply to this email directly or view it on GitHub
#3327 (comment).

Trott added a commit that referenced this pull request Oct 19, 2015
@Trott @rvagg
test: add Symbol test for assert.deepEqual()
ee2e641 
The existing test never ran because typeof Symbol === 'function' and not
'symbol'. We have Symbols now so remove the check and just run the test.

PR-URL: #3327
Reviewed-By: James M Snell <[email protected]>
@rvagg rvagg mentioned this pull request Oct 21, 2015
Merged
Trott added a commit that referenced this pull request Oct 26, 2015
@Trott @jasnell
test: add Symbol test for assert.deepEqual()
6305d26 
The existing test never ran because typeof Symbol === 'function' and not
'symbol'. We have Symbols now so remove the check and just run the test.

PR-URL: #3327
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Oct 26, 2015

Landed in v4.x-staging in 6305d26

@rvagg rvagg mentioned this pull request Oct 28, 2015
Closed
Trott added a commit that referenced this pull request Oct 29, 2015
@Trott @jasnell
test: add Symbol test for assert.deepEqual()
ea58fa0 
The existing test never ran because typeof Symbol === 'function' and not
'symbol'. We have Symbols now so remove the check and just run the test.

PR-URL: #3327
Reviewed-By: James M Snell <[email protected]>
@Trott Trott deleted the add-symbol-test branch January 13, 2022 22:29
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. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Trott @evanlucas @vkurchatkin @jasnell @MylesBorins @mscdex