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 known issue test for path parse issue #6229 #8293

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@jasnell
Member

jasnell commented Aug 27, 2016

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

known_issue

Description of change

Add known_issue test for #6229

Show outdated Hide outdated test/known_issues/test-path-parse.js
const assert = require('assert');
const path = require('path');
// Issue: https://github.com/nodejs/node/issues/6229

This comment has been minimized.

@cjihrig

cjihrig Aug 27, 2016

Contributor

Could you move this to the top, just under 'use strict;', and change Issue to Refs for consistency with the other known issue tests.

@cjihrig

cjihrig Aug 27, 2016

Contributor

Could you move this to the top, just under 'use strict;', and change Issue to Refs for consistency with the other known issue tests.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Aug 27, 2016

Contributor

LGTM with a comment.

Contributor

cjihrig commented Aug 27, 2016

LGTM with a comment.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 27, 2016

Member

Done. Also renamed the test file to be more specific.

Member

jasnell commented Aug 27, 2016

Done. Also renamed the test file to be more specific.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 27, 2016

Member

Nit: Two tests in the file, one for path.posix.parse('/foo/bar/') and another for path.win32.parse('\\foo\\bar\\').

Nit: Instead of asserting that the two results are not equal, maybe make the test a bit more strict by asserting that each one equals exactly what it is supposed to equal?

assert.strictEqual(parsed1, {root: '/', dir: '/foo', base: 'bar', ext: '', name: 'bar'});
assert.strictEqual(parsed2, {root: '/', dir: '/foo/bar', base: '', ext: '', name: ''});

If you're concerned that maybe we'll add another key to the object, you could do assertions specifically for the dir, base, and name properties.

Member

Trott commented Aug 27, 2016

Nit: Two tests in the file, one for path.posix.parse('/foo/bar/') and another for path.win32.parse('\\foo\\bar\\').

Nit: Instead of asserting that the two results are not equal, maybe make the test a bit more strict by asserting that each one equals exactly what it is supposed to equal?

assert.strictEqual(parsed1, {root: '/', dir: '/foo', base: 'bar', ext: '', name: 'bar'});
assert.strictEqual(parsed2, {root: '/', dir: '/foo/bar', base: '', ext: '', name: ''});

If you're concerned that maybe we'll add another key to the object, you could do assertions specifically for the dir, base, and name properties.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 27, 2016

Member

Updated.

Member

jasnell commented Aug 27, 2016

Updated.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 27, 2016

Member

LGTM

Member

Trott commented Aug 27, 2016

LGTM

jasnell added a commit that referenced this pull request Aug 29, 2016

test: add known issue test for path parse issue #6229
Refs: #6229
PR-URL: #8293
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 29, 2016

Member

Landed in 407069a
(Landed, then realized I'd completely missed doing a CI check... so retroactively... https://ci.nodejs.org/job/node-test-pull-request/3877/)

Member

jasnell commented Aug 29, 2016

Landed in 407069a
(Landed, then realized I'd completely missed doing a CI check... so retroactively... https://ci.nodejs.org/job/node-test-pull-request/3877/)

@jasnell jasnell closed this Aug 29, 2016

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

test: add known issue test for path parse issue #6229
Refs: nodejs#6229
PR-URL: nodejs#8293
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

test: add known issue test for path parse issue #6229
Refs: #6229
PR-URL: #8293
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

@ghost ghost referenced this pull request Sep 26, 2016

Closed

Malicious Path #84

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 30, 2016

Member

@jasnell backport to v4x?

Member

MylesBorins commented Sep 30, 2016

@jasnell backport to v4x?

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