-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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.
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
test: Update to const and use regex for assertions #9891
Conversation
|
||
connect({ | ||
host: 'localhost', | ||
port: common.PORT, | ||
localPort: 'foobar', | ||
}, 'localPort should be a number: foobar'); | ||
}, /"localPort" option should be a number: foobar/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: can you please add a leading ^
and a trailing $
to these regexes?
I can update. |
@lpinca please review 6cc35ac. Commit squashed to a single commit. |
6cc35ac
to
5cae0c3
Compare
|
||
connect({ | ||
host: 'localhost', | ||
port: common.PORT, | ||
localPort: 'foobar', | ||
}, 'localPort should be a number: foobar'); | ||
}, /^"localPort" option should be a number: foobar$/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Intregrisist you probably also have to add the TypeError
part or it will fail:
/^TypeError: "localPort" option should be a number: foobar$/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please CR: 98309e6 Tests are passing on my local. Sorry I missed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Intregrisist thanks! LGTM.
Use const over var. Assert error message with regex.
98309e6
to
f2cd619
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! thank you for the PR and for participating in the code-and-learn!
Use const over var. Assert error message with regex. PR-URL: #9891 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
Landed in 4a204ee. thank you! |
Use const over var. Assert error message with regex. PR-URL: #9891 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
Use const over var. Assert error message with regex. PR-URL: nodejs#9891 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
Use const over var. Assert error message with regex. PR-URL: nodejs#9891 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
Use const over var. Assert error message with regex. PR-URL: #9891 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
Use const over var. Assert error message with regex. PR-URL: #9891 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
Use const over var. Assert error message with regex. PR-URL: #9891 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
test
Description of change
Use const over var.
Assert error message with regex.
Warning
This PR changes Error message expectation and manipulating test to pass. Please be advised.