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 fs-assert-encoding's test #10913

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
@hiroppy
Member

hiroppy commented Jan 20, 2017

Check the error of assertEncoding.
Confirmed in every place being used.(a place where getOptions is used)
assetEncoding:

function assertEncoding(encoding) {

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

Show outdated Hide outdated test/parallel/test-fs-assert-encoding-error.js
new RegExp(`^Error: Unknown encoding: ${options}$`);
assert.throws(() => {
fs.readFile('path', options, () => {});

This comment has been minimized.

@cjihrig

cjihrig Jan 20, 2017

Contributor

Can you create a noop variable, give it the value of () => {}, then just reuse that.

@cjihrig

cjihrig Jan 20, 2017

Contributor

Can you create a noop variable, give it the value of () => {}, then just reuse that.

This comment has been minimized.

@hiroppy

hiroppy Jan 20, 2017

Member

Updated:)

@hiroppy

hiroppy Jan 20, 2017

Member

Updated:)

@cjihrig

One more small nit, but LGTM

Show outdated Hide outdated test/parallel/test-fs-assert-encoding-error.js
const options = 'test';
const noop = () => {};
const unknownEncodingMessage =
new RegExp(`^Error: Unknown encoding: ${options}$`);

This comment has been minimized.

@cjihrig

cjihrig Jan 20, 2017

Contributor

Since options never changes, you can just use test in the regex, and use a literal instead of the RegExp() constructor.

@cjihrig

cjihrig Jan 20, 2017

Contributor

Since options never changes, you can just use test in the regex, and use a literal instead of the RegExp() constructor.

This comment has been minimized.

@hiroppy

hiroppy Jan 21, 2017

Member

Updated.

@hiroppy

hiroppy Jan 21, 2017

Member

Updated.

test: add fs-assert-encoding's test
Check the error of `assertEncoding`.
Confirmed in every place being used.(a place where `getoptions` is used)
assetEncoding: https://github.com/nodejs/node/blob/521767c88605cb6481ea98f396924e55f9dd22f4/lib/internal/fs.js#L18
@cjihrig

This comment has been minimized.

Show comment
Hide comment
@lpinca

lpinca approved these changes Jan 25, 2017

@lpinca

This comment has been minimized.

Show comment
Hide comment
@lpinca

lpinca Jan 25, 2017

Member

Landed in aa4fe92.

Member

lpinca commented Jan 25, 2017

Landed in aa4fe92.

@lpinca lpinca closed this Jan 25, 2017

@lpinca

This comment has been minimized.

Show comment
Hide comment
@lpinca

lpinca Jan 25, 2017

Member

Ops I messed up.

Member

lpinca commented Jan 25, 2017

Ops I messed up.

@lpinca lpinca reopened this Jan 25, 2017

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Jan 25, 2017

Member

@lpinca you can still git push --force-with-lease to remove that commit again.

Member

gibfahn commented Jan 25, 2017

@lpinca you can still git push --force-with-lease to remove that commit again.

@lpinca

This comment has been minimized.

Show comment
Hide comment
@lpinca

lpinca Jan 25, 2017

Member

@gibfahn yes thanks, but I only closed the wrong PR by mistake :)

Member

lpinca commented Jan 25, 2017

@gibfahn yes thanks, but I only closed the wrong PR by mistake :)

@hiroppy

This comment has been minimized.

Show comment
Hide comment
@hiroppy

hiroppy Feb 1, 2017

Member

@lpinca Can I merge to master?

Member

hiroppy commented Feb 1, 2017

@lpinca Can I merge to master?

@lpinca

This comment has been minimized.

Show comment
Hide comment
Member

lpinca commented Feb 1, 2017

@abouthiroppy sure.

@hiroppy

This comment has been minimized.

Show comment
Hide comment
@hiroppy

This comment has been minimized.

Show comment
Hide comment
@hiroppy

hiroppy Feb 1, 2017

Member

test/freebsd's test failed... I think code is unrelated.

Member

hiroppy commented Feb 1, 2017

test/freebsd's test failed... I think code is unrelated.

@lpinca

This comment has been minimized.

Show comment
Hide comment
@lpinca

lpinca Feb 1, 2017

Member

Yes, there are some failures on freebsd10-64 but nothing seems to point to these changes. Go ahead.

Member

lpinca commented Feb 1, 2017

Yes, there are some failures on freebsd10-64 but nothing seems to point to these changes. Go ahead.

hiroppy added a commit that referenced this pull request Feb 1, 2017

test: add fs-assert-encoding's test
Check the error of `assertEncoding`.
Confirmed in every place being used.(a place where `getoptions` is used)
assetEncoding: https://github.com/nodejs/node/blob/521767c88605cb6481ea98f396924e55f9dd22f4/lib/internal/fs.js#L18

PR-URL: #10913
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@hiroppy

This comment has been minimized.

Show comment
Hide comment
@hiroppy

hiroppy Feb 1, 2017

Member

Landed in 9db8973.

Member

hiroppy commented Feb 1, 2017

Landed in 9db8973.

@hiroppy hiroppy closed this Feb 1, 2017

@hiroppy hiroppy deleted the hiroppy:feature/add-test-fs-assert-encoding-error branch Feb 1, 2017

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 2, 2017

test: add fs-assert-encoding's test
Check the error of `assertEncoding`.
Confirmed in every place being used.(a place where `getoptions` is used)
assetEncoding: https://github.com/nodejs/node/blob/521767c88605cb6481ea98f396924e55f9dd22f4/lib/internal/fs.js#L18

PR-URL: nodejs#10913
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

test: add fs-assert-encoding's test
Check the error of `assertEncoding`.
Confirmed in every place being used.(a place where `getoptions` is used)
assetEncoding: https://github.com/nodejs/node/blob/521767c88605cb6481ea98f396924e55f9dd22f4/lib/internal/fs.js#L18

PR-URL: nodejs#10913
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 7, 2017

Member

This is not landing cleanly on v6 or v4. Backport?

Member

jasnell commented Mar 7, 2017

This is not landing cleanly on v6 or v4. Backport?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 8, 2017

Member

Test fails on both v4 and v6 btw

Member

jasnell commented Mar 8, 2017

Test fails on both v4 and v6 btw

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