Skip to content

Conversation

@navulirs
Copy link
Contributor

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

37:5 error please use assert.strictEqual() instead of assert.equal()
39:5 error please use assert.strictEqual() instead of assert.equal()

37:5 error please use assert.strictEqual() instead of assert.equal() 39:5 error please use assert.strictEqual() instead of assert.equal()
@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@imyllerimyller added code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. child_process Issues and PRs related to the child_process subsystem. labels Dec 1, 2016
cat.on('exit',common.mustCall(function(status){
assert.strictEqual(0,status);
}));

Copy link
Contributor

Choose a reason for hiding this comment

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

There are two uses of assert.ok() in this file that could be greatly improved by changing to assert.strictEqual().

@Trott
Copy link
Member

Ping @sriluvan: Can you update this per the comment from @cjihrig? The values passed to assert.ok in this test are boolean values, so you should be able to use (for example) assert.strictEqual(cat.stdin.writable, true);.

@navulirs
Copy link
ContributorAuthor

Hi. I will get it in by tomorrow. Sorry for the delay again. Thanks.

@navulirs
Copy link
ContributorAuthor

I have fixed the issue and created the pull request. Again I'm sorry as I couldn't attend the request in time.

This is first time I'm getting change in outside the group work held in Austin. I'm hoping I followed the guidelines properly. Please let know if any thing not done correctly. I'm hoping this delay won't repeat. Thanks.

Copy link
Contributor

@sam-githubsam-github left a comment

Choose a reason for hiding this comment

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

Commit message is

37:5 error please use assert.strictEqual() instead of assert.equal() 39:5 error please use assert.strictEqual() instead of assert.equal() 

That isn't useful, please put in a human readable description.

Use assert.strictEqual() instead of assert.equal().

Would be good.

@navulirs
Copy link
ContributorAuthor

The message I added was "assert.ok to assert.strictEqual" - as per the change requested in this thread. - not the message in your email. I opened the commit message in amend node to check what is the comments area but I don't see the message in your comment. Can you direct me. Thanks.

@navulirs
Copy link
ContributorAuthor

The pull request with the suggested changes are in now. I'm not sure if this updated the original pull request or created new. Let me know if it is not update to original PR. Just want to know if I did the right steps.

@Trott
Copy link
Member

Closing in favor of #10420

@TrottTrott closed this Dec 24, 2016
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

child_processIssues and PRs related to the child_process subsystem.code-and-learnIssues related to the Code-and-Learn events and PRs submitted during the events.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@navulirs@Trott@sam-github@jasnell@cjihrig@imyller@nodejs-github-bot