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: make module testing stricter #11116

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@Trott
Member

Trott commented Feb 2, 2017

In test-module-loading-error:

  • Do not skip the rest of the test just because we are running on a
    platform for which the test does not know the expected system error
    message. Simply skip the message validation but run the remainder of
    the test.
  • Use assert.throws() in place of try/catch
  • Make checks more strict. Instead of partial string matches, match the
    entire string. Add check for Error name to at least do some validation
    in situations where we do not have the system error message.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test module

test: make module testing stricter
In test-module-loading-error:

* Do not skip the rest of the test just because we are running on a
  platform for which the test does not know the expected system error
  message. Simply skip the message validation but run the remainder of
  the test.
* Use assert.throws() in place of try/catch
* Make checks more strict. Instead of partial string matches, match the
  entire string. Add check for Error name to at least do some validation
  in situations where we do not have the system error message.

@Trott Trott added module test labels Feb 2, 2017

@Trott

This comment has been minimized.

Show comment
Hide comment
Member

Trott commented Feb 2, 2017

@mscdex mscdex referenced this pull request Feb 2, 2017

Closed

Bot failed to label PR #88

@danbev

danbev approved these changes Feb 2, 2017

@cjihrig

cjihrig approved these changes Feb 2, 2017

assert.throws(
() => { require('../fixtures/module-loading-error.node'); },
(e) => {
if (dlerror_msg && !dlerror_msg.some((msg) => e.message.includes(msg)))

This comment has been minimized.

@cjihrig

cjihrig Feb 2, 2017

Contributor

Would you be open to writing this check like this:

// Skip platforms where the error is not known.
if (!dlerror_msg)
  return true;

return e.name === 'Error' && dlerror_msg.some((msg) => e.message.includes(msg));

EDIT: Nevermind. We don't need to skip the e.name check on any platforms.

@cjihrig

cjihrig Feb 2, 2017

Contributor

Would you be open to writing this check like this:

// Skip platforms where the error is not known.
if (!dlerror_msg)
  return true;

return e.name === 'Error' && dlerror_msg.some((msg) => e.message.includes(msg));

EDIT: Nevermind. We don't need to skip the e.name check on any platforms.

@jasnell

jasnell approved these changes Feb 2, 2017

@lpinca

lpinca approved these changes Feb 4, 2017

jasnell added a commit that referenced this pull request Feb 4, 2017

test: make module testing stricter
In test-module-loading-error:

* Do not skip the rest of the test just because we are running on a
  platform for which the test does not know the expected system error
  message. Simply skip the message validation but run the remainder of
  the test.
* Use assert.throws() in place of try/catch
* Make checks more strict. Instead of partial string matches, match the
  entire string. Add check for Error name to at least do some validation
  in situations where we do not have the system error message.

PR-URL: #11116
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 4, 2017

Member

Landed in 8b04bc9

Member

jasnell commented Feb 4, 2017

Landed in 8b04bc9

@jasnell jasnell closed this Feb 4, 2017

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 5, 2017

test: make module testing stricter
In test-module-loading-error:

* Do not skip the rest of the test just because we are running on a
  platform for which the test does not know the expected system error
  message. Simply skip the message validation but run the remainder of
  the test.
* Use assert.throws() in place of try/catch
* Make checks more strict. Instead of partial string matches, match the
  entire string. Add check for Error name to at least do some validation
  in situations where we do not have the system error message.

PR-URL: nodejs#11116
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

test: make module testing stricter
In test-module-loading-error:

* Do not skip the rest of the test just because we are running on a
  platform for which the test does not know the expected system error
  message. Simply skip the message validation but run the remainder of
  the test.
* Use assert.throws() in place of try/catch
* Make checks more strict. Instead of partial string matches, match the
  entire string. Add check for Error name to at least do some validation
  in situations where we do not have the system error message.

PR-URL: nodejs#11116
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017

test: make module testing stricter
In test-module-loading-error:

* Do not skip the rest of the test just because we are running on a
  platform for which the test does not know the expected system error
  message. Simply skip the message validation but run the remainder of
  the test.
* Use assert.throws() in place of try/catch
* Make checks more strict. Instead of partial string matches, match the
  entire string. Add check for Error name to at least do some validation
  in situations where we do not have the system error message.

PR-URL: nodejs#11116
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 7, 2017

Member

Need backport PRs to land in v4 or v6

Member

jasnell commented Mar 7, 2017

Need backport PRs to land in v4 or v6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment