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: http2 add timeout no callback test case #16082

Conversation

trivikr
Copy link
Member

@trivikr trivikr commented Oct 8, 2017

This code change adds a test case where server.setTimeout is called without a callback
Refs: #14985

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

test, http2

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 8, 2017
@trivikr trivikr force-pushed the test-http2-server-settimeout-no-callback branch from 319538c to e3a7315 Compare October 8, 2017 03:38
@mscdex mscdex added the http2 Issues or PRs related to the http2 subsystem. label Oct 8, 2017
@watilde
Copy link
Member

watilde commented Oct 10, 2017

@refack
Copy link
Contributor

refack commented Oct 10, 2017

@trivikr thanks for contribution.

CI complains that there's a typo in test-http2-server-settimeout-no-callback.js:21:12:

not ok 926 parallel/test-http2-server-settimeout-no-callback
  ---
  duration_ms: 0.118
  severity: fail
  stack: |-
    assert.js:45
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 'Callback must be a function' === 'callback must be a function'
        at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:723:16)
        at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:517:15)
        at expectedException (assert.js:651:19)
        at innerThrows (assert.js:685:21)
        at Function.throws (assert.js:702:3)
        at Object.expectsError (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:745:12)
        at notFunctions.forEach (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-http2-server-settimeout-no-callback.js:21:12)
        at Array.forEach (<anonymous>)
        at verifyCallbacks (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-http2-server-settimeout-no-callback.js:20:16)
        at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-http2-server-settimeout-no-callback.js:35:3)
  ...

const invalidCallBackError = {
type: TypeError,
code: 'ERR_INVALID_CALLBACK',
message: 'callback must be a function'
Copy link
Contributor

Choose a reason for hiding this comment

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

Capital C in Callback.

@refack refack self-assigned this Oct 10, 2017
@refack
Copy link
Contributor

refack commented Oct 10, 2017

P.S. @trivikr now that you're a "seasoned" contributor, you might want to pick up a few common practices:

  1. You can quickly run a changed test with node test/parallel/test-http2-server-settimeout-no-callback.js and get immediate feedback. (just make sure your node binary is recent enough, either build localy or get the latest nightly from https://nodejs.org/download/nightly/)
  2. Run the linter locally (make lint-js)
  3. For changes in the core code, you should build and run the test suite locally

This and more information in https://github.com/nodejs/node/blob/master/CONTRIBUTING.md

Hope to see you contribute even more in the future 😉

@trivikr trivikr force-pushed the test-http2-server-settimeout-no-callback branch from e3a7315 to 7c484af Compare October 10, 2017 16:41
@refack
Copy link
Contributor

refack commented Oct 10, 2017

@trivikr
Copy link
Member Author

trivikr commented Oct 10, 2017

Thanks @refack
I've been using the following command to run tests
python tools/test.py -J --mode=release parallel/test-http2-server-settimeout-no-callback
In this case I would have forgotten to run tests after adding message in invalidCallBackError, my bad.

The python command is more visual, it shows success and failure using + and -

@mcollina
Copy link
Member

@mcollina mcollina assigned mcollina and unassigned refack Oct 16, 2017
@mcollina
Copy link
Member

Landing.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina
Copy link
Member

Landed as e2015b5.

@mcollina mcollina closed this Oct 16, 2017
mcollina pushed a commit that referenced this pull request Oct 16, 2017
Refs: #14985
PR-URL: #16082
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@trivikr trivikr deleted the test-http2-server-settimeout-no-callback branch October 17, 2017 02:51
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 18, 2017
Refs: nodejs/node#14985
PR-URL: nodejs/node#16082
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
Refs: #14985
PR-URL: #16082
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants