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

Closed
wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell 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

const assert = require('assert');
const path = require('path');

// Issue: https://github.com/nodejs/node/issues/6229
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

cjihrig commented Aug 27, 2016

LGTM with a comment.

@jasnell
Copy link
Member Author

jasnell commented Aug 27, 2016

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

@Trott
Copy link
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
Copy link
Member Author

jasnell commented Aug 27, 2016

Updated.

@Trott
Copy link
Member

Trott commented Aug 27, 2016

LGTM

jasnell added a commit that referenced this pull request Aug 29, 2016
Refs: #6229
PR-URL: #8293
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jasnell
Copy link
Member Author

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 mentioned this pull request Sep 6, 2016
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Sep 8, 2016
Refs: nodejs#6229
PR-URL: nodejs#8293
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Sep 9, 2016
Refs: #6229
PR-URL: #8293
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@ghost ghost mentioned this pull request Sep 26, 2016
@MylesBorins
Copy link
Contributor

@jasnell backport to v4x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants