test: add check in test-signal-handler #8248

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@Trott
Member

Trott commented Aug 24, 2016

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

test process

Description of change
  • Check that the removed listener is not called.
  • Opportunistic == -> === change.
test: add check in test-signal-handler
* Check that the removed listener is not called.
* Opportunistic `==` -> `===` change.
process.kill(process.pid, 'SIGUSR1');
}
}, 1);
// Test on condition where a watcher for SIGNAL
// has been previously registered, and `process.listeners(SIGNAL).length === 1`
-process.on('SIGHUP', function() {});
+process.on('SIGHUP', function() { common.fail('should not run'); });

This comment has been minimized.

@yorkie

yorkie Aug 24, 2016

Member

Wrap it with common.mustCall(), tho?

@yorkie

yorkie Aug 24, 2016

Member

Wrap it with common.mustCall(), tho?

This comment has been minimized.

@Trott

Trott Aug 24, 2016

Member

If the function is called, the test has failed. It would be wrapped in a 'common.mustNotCall()' if we had one.

@Trott

Trott Aug 24, 2016

Member

If the function is called, the test has failed. It would be wrapped in a 'common.mustNotCall()' if we had one.

This comment has been minimized.

@jasnell

jasnell Aug 24, 2016

Member

I would simply make this

process.on('SIGHUP', common.fail)

And just do without the should not run text.

@jasnell

jasnell Aug 24, 2016

Member

I would simply make this

process.on('SIGHUP', common.fail)

And just do without the should not run text.

This comment has been minimized.

@Trott

Trott Aug 24, 2016

Member

@jasnell Done that way, the AssertionError looks like this:

AssertionError: null undefined null
    at process.exports.fail (/Users/trott/io.js/test/common.js:438:10)
    at emitNone (events.js:91:20)
    at process.emit (events.js:185:7)
    at Signal.wrap.onsignal (internal/process.js:199:44)

There are two things I don't like about that:

  • The stack trace does not show where the actual problem is. It does not mention the test file at all.
  • null undefined null: What?!

In contrast, done the way it is in this PR, it looks like this:

AssertionError: should not run
    at Object.exports.fail (/Users/trott/io.js/test/common.js:438:10)
    at process.<anonymous> (/Users/trott/io.js/test/parallel/test-signal-handler.js:32:42)
    at emitNone (events.js:91:20)
    at process.emit (events.js:185:7)
    at Signal.wrap.onsignal (internal/process.js:199:44)

The stack trace includes the name of the test file and the line that resulted in the failure.

@Trott

Trott Aug 24, 2016

Member

@jasnell Done that way, the AssertionError looks like this:

AssertionError: null undefined null
    at process.exports.fail (/Users/trott/io.js/test/common.js:438:10)
    at emitNone (events.js:91:20)
    at process.emit (events.js:185:7)
    at Signal.wrap.onsignal (internal/process.js:199:44)

There are two things I don't like about that:

  • The stack trace does not show where the actual problem is. It does not mention the test file at all.
  • null undefined null: What?!

In contrast, done the way it is in this PR, it looks like this:

AssertionError: should not run
    at Object.exports.fail (/Users/trott/io.js/test/common.js:438:10)
    at process.<anonymous> (/Users/trott/io.js/test/parallel/test-signal-handler.js:32:42)
    at emitNone (events.js:91:20)
    at process.emit (events.js:185:7)
    at Signal.wrap.onsignal (internal/process.js:199:44)

The stack trace includes the name of the test file and the line that resulted in the failure.

This comment has been minimized.

@jasnell

jasnell Aug 24, 2016

Member

null undefined null is going to be my new catchphrase... sigh.. looks like we should tweak the output for common.fail

@jasnell

jasnell Aug 24, 2016

Member

null undefined null is going to be my new catchphrase... sigh.. looks like we should tweak the output for common.fail

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Aug 24, 2016

Contributor

LGTM

Contributor

cjihrig commented Aug 24, 2016

LGTM

@Trott

This comment has been minimized.

Show comment
Hide comment

Trott added a commit to Trott/io.js that referenced this pull request Aug 26, 2016

test: add check in test-signal-handler
* Check that the removed listener is not called.
* Opportunistic `==` -> `===` change.

PR-URL: nodejs#8248
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 26, 2016

Member

Landed in a6d53c6

Member

Trott commented Aug 26, 2016

Landed in a6d53c6

@Trott Trott closed this Aug 26, 2016

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

test: add check in test-signal-handler
* Check that the removed listener is not called.
* Opportunistic `==` -> `===` change.

PR-URL: nodejs#8248
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

test: add check in test-signal-handler
* Check that the removed listener is not called.
* Opportunistic `==` -> `===` change.

PR-URL: #8248
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 30, 2016

test: add check in test-signal-handler
* Check that the removed listener is not called.
* Opportunistic `==` -> `===` change.

PR-URL: #8248
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins added a commit that referenced this pull request Oct 10, 2016

test: add check in test-signal-handler
* Check that the removed listener is not called.
* Opportunistic `==` -> `===` change.

PR-URL: #8248
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

rvagg added a commit that referenced this pull request Oct 18, 2016

test: add check in test-signal-handler
* Check that the removed listener is not called.
* Opportunistic `==` -> `===` change.

PR-URL: #8248
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins added a commit that referenced this pull request Oct 26, 2016

test: add check in test-signal-handler
* Check that the removed listener is not called.
* Opportunistic `==` -> `===` change.

PR-URL: #8248
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@MylesBorins MylesBorins referenced this pull request Oct 26, 2016

Closed

V4.6.2 proposal #9298

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