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: add check in test-signal-handler #8248

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott 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.

* Check that the removed listener is not called.
* Opportunistic `==` -> `===` change.
@Trott Trott added test Issues and PRs related to the tests. process Issues and PRs related to the process subsystem. labels Aug 24, 2016
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'); });
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrap it with common.mustCall(), tho?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I would simply make this

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

And just do without the should not run text.

Copy link
Member Author

@Trott Trott Aug 24, 2016

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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

@cjihrig
Copy link
Contributor

cjihrig commented Aug 24, 2016

LGTM

@Trott
Copy link
Member Author

Trott commented Aug 24, 2016

Trott added a commit to Trott/io.js that referenced this pull request Aug 26, 2016
* Check that the removed listener is not called.
* Opportunistic `==` -> `===` change.

PR-URL: nodejs#8248
Reviewed-By: Colin Ihrig <[email protected]>
@Trott
Copy link
Member Author

Trott commented Aug 26, 2016

Landed in a6d53c6

@Trott Trott closed this Aug 26, 2016
@Fishrock123 Fishrock123 mentioned this pull request Sep 6, 2016
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Sep 8, 2016
* Check that the removed listener is not called.
* Opportunistic `==` -> `===` change.

PR-URL: nodejs#8248
Reviewed-By: Colin Ihrig <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Sep 9, 2016
* Check that the removed listener is not called.
* Opportunistic `==` -> `===` change.

PR-URL: #8248
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
* Check that the removed listener is not called.
* Opportunistic `==` -> `===` change.

PR-URL: #8248
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
* Check that the removed listener is not called.
* Opportunistic `==` -> `===` change.

PR-URL: #8248
Reviewed-By: Colin Ihrig <[email protected]>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
* Check that the removed listener is not called.
* Opportunistic `==` -> `===` change.

PR-URL: #8248
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
* Check that the removed listener is not called.
* Opportunistic `==` -> `===` change.

PR-URL: #8248
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
@Trott Trott deleted the check-remove branch January 13, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants