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: removed string literal on assert.strictEqual #16014

Closed
wants to merge 1 commit into from
Closed

test: removed string literal on assert.strictEqual #16014

wants to merge 1 commit into from

Conversation

Saeed-Navarik
Copy link
Contributor

@Saeed-Navarik Saeed-Navarik commented Oct 6, 2017 •
edited by tniessen
Loading

removed string literal argument to function call strictEqual
so that default error message is printed

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@Saeed-Navarik Saeed-Navarik changed the title test: removed msg on assert.strictEqual on test-tls-pfx-gh-5100-regr test: removed msg on assert.strictEqual Oct 6, 2017
@Saeed-Navarik Saeed-Navarik changed the title test: removed msg on assert.strictEqual test: removed string literal on assert.strictEqual Oct 6, 2017
@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Oct 6, 2017
@joyeecheung
Copy link
Member

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

@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 7, 2017
gibfahn
gibfahn approved these changes Oct 7, 2017
View reviewed changes
Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

One suggestion, but LGTM either way.

test/parallel/test-tls-pfx-gh-5100-regr.js
@@ -19,8 +19,7 @@ const server = tls.createServer({
}, common.mustCall(function(c) {
assert.strictEqual(
c.authorizationError,
null,
'authorizationError must be null'
null
);
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be reformatted to be on one line like:

assert.strictEqual(c.authorizationError, null);

If you could push a commit to your branch that makes this change then that would be great.

@joyeecheung
Copy link
Member

joyeecheung commented Oct 7, 2017 •
edited
Loading

Fixed @gibfahn 's comments and landed in 5843b2c, thanks for your contribution and welcome to Node.js!

@joyeecheung joyeecheung closed this Oct 7, 2017
joyeecheung pushed a commit that referenced this pull request Oct 7, 2017
@Saeed-Navarik @joyeecheung
test: remove messages in assert.strictEqual
5843b2c 
PR-URL: #16014
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request Oct 11, 2017
@Saeed-Navarik @MylesBorins
test: remove messages in assert.strictEqual
1d7e1c0 
PR-URL: #16014
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@MylesBorins MylesBorins mentioned this pull request Oct 11, 2017
Merged
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 12, 2017
@Saeed-Navarik @addaleax
test: remove messages in assert.strictEqual
81844b7 
PR-URL: nodejs/node#16014
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request Oct 17, 2017
@Saeed-Navarik @MylesBorins
test: remove messages in assert.strictEqual
c81b425 
PR-URL: #16014
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Oct 18, 2017
@Saeed-Navarik @MylesBorins
test: remove messages in assert.strictEqual
c308a30 
PR-URL: #16014
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@Saeed-Navarik @MylesBorins
test: remove messages in assert.strictEqual
968cc44 
PR-URL: #16014
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@benjamingr benjamingr benjamingr approved these changes

Assignees
No one assigned
Labels
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. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@Saeed-Navarik @joyeecheung @benjamingr @cjihrig @tniessen @gibfahn @mscdex @MylesBorins @Trott @nodejs-github-bot