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: change var declarations, add mustCall check #9962

Closed
wants to merge 3 commits into from

Conversation

danielgsims
Copy link

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

test

Description of change

In this test, I changed the var declarations to be either a let or a
const. For some of the callbacks, I added a mustCall check to ensure
that the functions have run.

In this test, I changed the var declarations to be either a let or a
const. For some of the callbacks, I added a mustCall check to ensure
that the functions have run.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@danielgsims
Copy link
Author

This is from the code and learn at NINA

@imyller imyller added code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. cluster Issues and PRs related to the cluster subsystem. labels Dec 1, 2016
@mscdex mscdex added the net Issues and PRs related to the net subsystem. label Dec 1, 2016

process.once('exit', function() {
console.log('runs');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please remove this console.log statement here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops, sure.

@danielgsims
Copy link
Author

@julianduque - all set

@Trott
Copy link
Member

Trott commented Dec 4, 2016

Thanks for the contribution! If it's not too much trouble, can you switch the occurrences of assert.equal() in the test to assert.strictEqual()?

function socketConnected() {
if (++cbcalls === 2)
process.send('handle', socket);
}

var server = net.createServer(function(c) {
process.once('message', function(msg) {
let server = net.createServer(function(c) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this be const?

I changed the let to a const declaration, and edited the equal
assertions to a strict assertion.
@danielgsims
Copy link
Author

Here you go, let me know if there's anything else.

@Trott
Copy link
Member

Trott commented Dec 22, 2016

Ping @cjihrig: Looks like your comment has been addressed. If so, can you update your review?

@Trott
Copy link
Member

Trott commented Dec 22, 2016

Trott pushed a commit to Trott/io.js that referenced this pull request Dec 23, 2016
In this test, I changed the var declarations to be either a let or a
const. For some of the callbacks, I added a mustCall check to ensure
that the functions have run. I also changed assert.equal() to
assert.strictEqual().

PR-URL: nodejs#9962
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott
Copy link
Member

Trott commented Dec 23, 2016

Landed in 9fd79c9.
Thanks for the contribution! 🎉

If you or anyone else wants a good second commit, this can possibly be further refactored to get rid of the called variable and the process.on('exit', ...) handler by using common.mustCall().

@Trott Trott closed this Dec 23, 2016
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
In this test, I changed the var declarations to be either a let or a
const. For some of the callbacks, I added a mustCall check to ensure
that the functions have run. I also changed assert.equal() to
assert.strictEqual().

PR-URL: #9962
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
In this test, I changed the var declarations to be either a let or a
const. For some of the callbacks, I added a mustCall check to ensure
that the functions have run. I also changed assert.equal() to
assert.strictEqual().

PR-URL: #9962
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
In this test, I changed the var declarations to be either a let or a
const. For some of the callbacks, I added a mustCall check to ensure
that the functions have run. I also changed assert.equal() to
assert.strictEqual().

PR-URL: #9962
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
In this test, I changed the var declarations to be either a let or a
const. For some of the callbacks, I added a mustCall check to ensure
that the functions have run. I also changed assert.equal() to
assert.strictEqual().

PR-URL: #9962
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
In this test, I changed the var declarations to be either a let or a
const. For some of the callbacks, I added a mustCall check to ensure
that the functions have run. I also changed assert.equal() to
assert.strictEqual().

PR-URL: #9962
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
In this test, I changed the var declarations to be either a let or a
const. For some of the callbacks, I added a mustCall check to ensure
that the functions have run. I also changed assert.equal() to
assert.strictEqual().

PR-URL: #9962
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This was referenced Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
In this test, I changed the var declarations to be either a let or a
const. For some of the callbacks, I added a mustCall check to ensure
that the functions have run. I also changed assert.equal() to
assert.strictEqual().

PR-URL: #9962
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
In this test, I changed the var declarations to be either a let or a
const. For some of the callbacks, I added a mustCall check to ensure
that the functions have run. I also changed assert.equal() to
assert.strictEqual().

PR-URL: #9962
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants