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 tests for options.fs in fs streams #33185

Conversation

julianduque
Copy link
Contributor

This add tests for the new options.fs override methods in the fs Stream operations

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 1, 2020
Copy link
Member

@edsadr edsadr left a comment

Choose a reason for hiding this comment

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

Just a suggestion but looks nice 🙂

test/parallel/test-fs-stream-fs-options.js Outdated Show resolved Hide resolved
test/parallel/test-fs-stream-fs-options.js Outdated Show resolved Hide resolved
@julianduque julianduque force-pushed the julianduque/test-fs-streams-fs-options branch from 6e16e76 to e1e860a Compare May 1, 2020 02:57
@julianduque julianduque requested a review from edsadr May 1, 2020 02:58
Copy link
Member

@edsadr edsadr left a comment

Choose a reason for hiding this comment

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

LGTM

@julianduque julianduque force-pushed the julianduque/test-fs-streams-fs-options branch from e1e860a to c671654 Compare May 1, 2020 14:34
Copy link
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

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

The only exception thrown is here https://github.com/nodejs/node/blob/master/lib/internal/fs/streams.js#L83, the rest of them are not being touched.

@julianduque julianduque force-pushed the julianduque/test-fs-streams-fs-options branch from c671654 to 258a8ca Compare May 1, 2020 15:04
@julianduque
Copy link
Contributor Author

@juanarbol addressed your comments, all branches are being tested now.

@julianduque julianduque requested a review from juanarbol May 1, 2020 15:06
@julianduque julianduque force-pushed the julianduque/test-fs-streams-fs-options branch from 258a8ca to 41415f4 Compare May 1, 2020 15:18
@julianduque julianduque requested a review from juanarbol May 1, 2020 15:18
Copy link
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

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

Looks amazing to me.

@juanarbol juanarbol added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 4, 2020
@nodejs-github-bot
Copy link
Collaborator

juanarbol pushed a commit that referenced this pull request May 4, 2020
PR-URL: #33185
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@juanarbol
Copy link
Member

Landed in 39ff647 🎉

@juanarbol juanarbol closed this May 4, 2020
codebytere pushed a commit that referenced this pull request May 7, 2020
PR-URL: #33185
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@codebytere codebytere mentioned this pull request May 18, 2020
codebytere pushed a commit that referenced this pull request Jun 7, 2020
PR-URL: #33185
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@codebytere codebytere mentioned this pull request Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants