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

Improve test-tls-key-mismatch.js #12011

Closed
wants to merge 1 commit into from
Closed

Improve test-tls-key-mismatch.js #12011

wants to merge 1 commit into from

Conversation

jfmercer
Copy link
Contributor

@jfmercer jfmercer commented Mar 23, 2017 •
edited
Loading

Improves test/parallel/test-tls-key-mismatch.js by providing a regex to validate the error message.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 23, 2017
@jfmercer
Copy link
Contributor Author

@Trott ⬆️

@vsemozhetbyt vsemozhetbyt added the tls Issues and PRs related to the tls subsystem. label Mar 23, 2017
@vsemozhetbyt
Copy link
Contributor

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

sam-github
sam-github reviewed Mar 23, 2017
View reviewed changes
test/parallel/test-tls-key-mismatch.js
@@ -29,6 +29,9 @@ if (!common.hasCrypto) {
const assert = require('assert');
const tls = require('tls');
const fs = require('fs');
const errorMessageRegex = new RegExp('^Error: error:0B080074:x509 ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible write this in a way that involves less line-breaking? Its all unused whitespace ATM :-(. Maybe put the msg string as its own line, const msg = 'error:0B....';? Or wrapping the entire argument to RegExp() to the next line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sam-github The error message is too long to fit within 80 cols. I'll see if I can condense it to two lines instead of three & then update the PR accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sam-github FWIW, the 80 col rule came from eslint, not me.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks. if that is as good as it can be, so it goes, but thanks for trying

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sam-github I've condensed it to two lines & updated the PR.

@jfmercer jfmercer force-pushed the improve-test-tls-key-mismatch branch from aa2617f to 502f9f3 Compare March 23, 2017 23:19
@vsemozhetbyt
Copy link
Contributor

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

Trott
Trott approved these changes Mar 24, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM

@fhinkel
Copy link
Member

fhinkel commented Mar 26, 2017

Thanks. Landed in c13dda1

@fhinkel fhinkel closed this Mar 26, 2017
fhinkel pushed a commit that referenced this pull request Mar 26, 2017
@jfmercer @fhinkel
test: add regex for expected error message
c13dda1 
Provide a regex to validate the error message.

PR-URL: #12011
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
@jfmercer @MylesBorins
test: add regex for expected error message
1c0435b 
Provide a regex to validate the error message.

PR-URL: #12011
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 28, 2017
Merged
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
Merged
2 tasks
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
@jfmercer @MylesBorins
test: add regex for expected error message
cd85dcc 
Provide a regex to validate the error message.

PR-URL: #12011
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@jfmercer @MylesBorins
test: add regex for expected error message
c65de59 
Provide a regex to validate the error message.

PR-URL: #12011
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@jfmercer @andrew749
test: add regex for expected error message
58e4b60 
Provide a regex to validate the error message.

PR-URL: nodejs/node#12011
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hiroppy hiroppy hiroppy approved these changes

@cjihrig cjihrig cjihrig approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

@fhinkel fhinkel fhinkel approved these changes

@sam-github sam-github sam-github approved these changes

Assignees
No one assigned
Labels
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.