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.

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: changed error message validator #14443

Closed
wants to merge 1 commit into from

Conversation

pratik0509
Copy link
Contributor

@pratik0509 pratik0509 commented Jul 24, 2017

Replaced TypeError with RegEx to match the exact error message in file
test/addons-napi/test_properties/test.js. The RegEx will check the
validity of the error being thrown.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Jul 24, 2017
@vitkarpov
Copy link
Contributor

Hey, @pratik0509 ! I'm just curious, why do we need this change? I mean, is the current test weak or something?

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jul 24, 2017

@vitkarpov This is recommended in the test guide.

CI: https://ci.nodejs.org/job/node-test-pull-request/9317/ (green)

Replaced TypeError with RegEx to match the exact error message in file
test/addons-napi/test_properties/test.js. The RegEx will check the
validity of the error being thrown.
@vitkarpov
Copy link
Contributor

@vsemozhetbyt gotcha! ;)

@Trott
Copy link
Member

Trott commented Jul 24, 2017

Providing a little more detail for @vitkarpov if they're curious--ignore if you don't care. :-D

It's recommended in the test guide because, for the time being at least, Node.js treats changes in error messages as breaking changes. So we want tests to catch those changes. They are treated as breaking changes because we know people parse the error messages. It would be nice to get to an error message system where nobody has to do that, but we're not there yet. (Movement is happening in that direction. Hopefully we get there sooner rather than later. We'll see.)

@addaleax
Copy link
Member

LGTM, thank you!

Landed in 7849b52

@addaleax addaleax closed this Jul 26, 2017
addaleax pushed a commit that referenced this pull request Jul 26, 2017
Replaced TypeError with RegEx to match the exact error message in file
test/addons-napi/test_properties/test.js. The RegEx will check the
validity of the error being thrown.

PR-URL: #14443
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@pratik0509
Copy link
Contributor Author

Thank you nodeJS community and especially @Trott for being patient and helping me.

addaleax pushed a commit that referenced this pull request Jul 27, 2017
Replaced TypeError with RegEx to match the exact error message in file
test/addons-napi/test_properties/test.js. The RegEx will check the
validity of the error being thrown.

PR-URL: #14443
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax addaleax mentioned this pull request Aug 2, 2017
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
Replaced TypeError with RegEx to match the exact error message in file
test/addons-napi/test_properties/test.js. The RegEx will check the
validity of the error being thrown.

PR-URL: nodejs#14443
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
Replaced TypeError with RegEx to match the exact error message in file
test/addons-napi/test_properties/test.js. The RegEx will check the
validity of the error being thrown.

Backport-PR-URL: #19447
PR-URL: #14443
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants