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: expand test coverage of events.js and fs.js #10947

Closed
wants to merge 2 commits into from

Conversation

vinimdocarmo
Copy link
Contributor

@vinimdocarmo vinimdocarmo commented Jan 22, 2017

events.js:

  • test else path in emitMany function
  • test calling removeAllListeners() in a event emitter instance
    with no events at all
  • test calling removeListener() passing a event type that does
    not exist
  • test calling eventNames() in a event emitter instance
    with no events at all

Refs: https://coverage.nodejs.org/coverage-ba776b3a56642d4c/root/events.js.html

fs.js:

  • test reading a file passing a not valid enconding

Refs: https://coverage.nodejs.org/coverage-067be658f966dafe/root/internal/fs.js.html

Checklist
  • make -j4 test (UNIX)
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. dont-land-on-v7.x labels Jan 22, 2017
@mscdex mscdex added events Issues and PRs related to the events subsystem / EventEmitter. and removed dont-land-on-v7.x labels Jan 22, 2017
@vinimdocarmo vinimdocarmo changed the title test: extend test coverage of events.js test: expand test coverage of events.js and file.js Jan 23, 2017
@vinimdocarmo vinimdocarmo changed the title test: expand test coverage of events.js and file.js test: expand test coverage of events.js and fs.js Jan 23, 2017
@jasnell
Copy link
Member

jasnell commented Jan 23, 2017

It would be better to split these into two separate commits, one for each subsystem for which coverage is being improved.

@vinimdocarmo
Copy link
Contributor Author

@jasnell, I'll do it.

* test else path in emitMany function
* test calling removeAllListeners() in a event emitter instance
  with no events at all
* test calling removeListener() passing a event type that does
  not exist
* test calling eventNames() in a event emitter instance
  with no events at all

Refs: https://coverage.nodejs.org/coverage-ba776b3a56642d4c/root/events.js.html
@vinimdocarmo
Copy link
Contributor Author

@jasnell, done!

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM. Great to see commits improving our code coverage :)

@jasnell
Copy link
Member

jasnell commented Jan 24, 2017

@vinimdocarmo
Copy link
Contributor Author

Any updates about this PR?

@gibfahn
Copy link
Member

gibfahn commented Jan 26, 2017

@vinimdocarmo Thanks for the ping, this PR should be good to go, as the CI failure seems unrelated:

https://ci.nodejs.org/job/node-test-binary-arm/6047/RUN_SUBSET=5,label=pi1-raspbian-wheezy/console

not ok 110 parallel/test-npm-install
  ---
  duration_ms: 180.181
  severity: fail
  stack: |-
    timeout
  ...

I'll land this tomorrow if no-one beats me to it.

@gibfahn
Copy link
Member

gibfahn commented Jan 27, 2017

Landed in b19334e and bee83e0

@gibfahn gibfahn closed this Jan 27, 2017
gibfahn pushed a commit that referenced this pull request Jan 27, 2017
* test else path in emitMany function
* test calling removeAllListeners() in a event emitter instance
  with no events at all
* test calling removeListener() passing a event type that does
  not exist
* test calling eventNames() in a event emitter instance
  with no events at all

Refs: https://coverage.nodejs.org/coverage-ba776b3a56642d4c/root/events.js.html
PR-URL: #10947
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
gibfahn pushed a commit that referenced this pull request Jan 27, 2017
* test reading a file passing a not valid enconding

Refs: https://coverage.nodejs.org/coverage-067be658f966dafe/root/internal/fs.js.html
PR-URL: #10947
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@vinimdocarmo vinimdocarmo deleted the extend-tests branch January 30, 2017 12:02
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
* test else path in emitMany function
* test calling removeAllListeners() in a event emitter instance
  with no events at all
* test calling removeListener() passing a event type that does
  not exist
* test calling eventNames() in a event emitter instance
  with no events at all

Refs: https://coverage.nodejs.org/coverage-ba776b3a56642d4c/root/events.js.html
PR-URL: #10947
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
* test reading a file passing a not valid enconding

Refs: https://coverage.nodejs.org/coverage-067be658f966dafe/root/internal/fs.js.html
PR-URL: #10947
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@italoacasas italoacasas mentioned this pull request Jan 31, 2017
jasnell pushed a commit that referenced this pull request Mar 8, 2017
* test reading a file passing a not valid enconding

Refs: https://coverage.nodejs.org/coverage-067be658f966dafe/root/internal/fs.js.html
PR-URL: #10947
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 8, 2017

Both commits landed clean in v6. Only one (the fs.js tests) landed clean in v4. Will need a backport to land the other.

jasnell pushed a commit that referenced this pull request Mar 8, 2017
* test else path in emitMany function
* test calling removeAllListeners() in a event emitter instance
  with no events at all
* test calling removeListener() passing a event type that does
  not exist
* test calling eventNames() in a event emitter instance
  with no events at all

Refs: https://coverage.nodejs.org/coverage-ba776b3a56642d4c/root/events.js.html
PR-URL: #10947
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 8, 2017
* test reading a file passing a not valid enconding

Refs: https://coverage.nodejs.org/coverage-067be658f966dafe/root/internal/fs.js.html
PR-URL: #10947
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
* test else path in emitMany function
* test calling removeAllListeners() in a event emitter instance
  with no events at all
* test calling removeListener() passing a event type that does
  not exist
* test calling eventNames() in a event emitter instance
  with no events at all

Refs: https://coverage.nodejs.org/coverage-ba776b3a56642d4c/root/events.js.html
PR-URL: #10947
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
* test reading a file passing a not valid enconding

Refs: https://coverage.nodejs.org/coverage-067be658f966dafe/root/internal/fs.js.html
PR-URL: #10947
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
* test reading a file passing a not valid enconding

Refs: https://coverage.nodejs.org/coverage-067be658f966dafe/root/internal/fs.js.html
PR-URL: #10947
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants