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

Closed
wants to merge 3 commits into from
Closed

test: change var declarations, add mustCall check #9962

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.

test: change var declarations, add mustCall check
3b7ece6 
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
julianduque
julianduque reviewed Dec 1, 2016
View reviewed changes
test/parallel/test-cluster-net-send.js

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()?

cjihrig
cjihrig requested changes Dec 5, 2016
View reviewed changes
test/parallel/test-cluster-net-send.js
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?

test: change let declaration, assertions
f139445 
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

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

Trott pushed a commit to Trott/io.js that referenced this pull request Dec 23, 2016
@Trott
test: change var declarations, add mustCall check
9fd79c9 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@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 evanlucas mentioned this pull request Jan 3, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
@evanlucas
test: change var declarations, add mustCall check
78376e6 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
@evanlucas
test: change var declarations, add mustCall check
bee7d7e 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@MylesBorins
test: change var declarations, add mustCall check
8eb8c0f 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@MylesBorins
test: change var declarations, add mustCall check
cd08da8 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@MylesBorins
test: change var declarations, add mustCall check
4960a3e 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@MylesBorins
test: change var declarations, add mustCall check
1c21166 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
This was referenced Jan 24, 2017
Merged
Merged
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
@MylesBorins
test: change var declarations, add mustCall check
390cab8 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
@MylesBorins
test: change var declarations, add mustCall check
122917d 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@aliscco aliscco mentioned this pull request Jun 21, 2023
Open
This was referenced Jul 1, 2023
Open
Open
This was referenced Jul 8, 2023
Open
Open
@leonardoadame leonardoadame mentioned this pull request Sep 30, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julianduque julianduque julianduque left review comments

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@danielgsims @Trott @jasnell @julianduque @cjihrig @mscdex @MylesBorins @imyller @nodejs-github-bot