Skip to content

Conversation

navulirs
Copy link
Contributor

@navulirs navulirs commented Dec 1, 2016

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-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@imyller imyller 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

Trott commented Dec 22, 2016

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
Contributor Author

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

@navulirs
Copy link
Contributor Author

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-github sam-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
Contributor Author

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
Contributor Author

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

Trott commented Dec 24, 2016

Closing in favor of #10420

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

Labels

child_process Issues and PRs related to the child_process subsystem. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants