10000 Fix Windows compatibility by brandon93s · Pull Request #78 · sindresorhus/cpy · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix Windows compatibility #78

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 2, 2020
Merged

Fix Windows compatibility #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
os:
- linux
- osx
- windows
language: node_js
node_js:
- '12'
Expand Down
5 changes: 3 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ const preprocessSourcePath = (source, options) => path.resolve(options.cwd ? opt

const preprocessDestinationPath = (source, destination, options) => {
let basename = path.basename(source);
const dirname = path.dirname(source);

if (typeof options.rename === 'string') {
basename = options.rename;
Expand All @@ -52,7 +51,9 @@ const preprocessDestinationPath = (source, destination, options) => {
}

if (options.parents) {
return path.join(destination, dirname, basename);
const dirname = path.dirname(source);
const parsedDirectory = path.parse(dirname);
return path.join(destination, dirname.replace(parsedDirectory.root, path.sep), basename);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

destination and dirname can both be absolute paths. When this occurs you end up with the following:

# MacOS & Linux
/tmp/example + /tmp/example/cwd => /tmp/example/tmp/example/cwd

# Windows
C:\\tmp\\example + C:\\tmp\\example\\cwd => C:\\tmp\\example\\C:\\tmp\\example\\cwd

The resulting Windows path is invalid. This strips the root ( C:\\ ) from the directory path before joining by replacing it with the path seperator. This will be a NOOP on non-Windows.

Copy link
Owner

Choose a reason for hiding this comment

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

Good catch. Would you be able to add a test for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is covered by the 'path structure' test which was failing prior to this fix.

}

return path.join(destination, basename);
Expand Down
5 changes: 3 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,10 @@
"p-map": "^3.0.0"
},
"devDependencies": {
"ava": "^2.1.0",
"ava": "^3.12.1",
"proxyquire": "^2.1.3",
"rimraf": "^3.0.0",
"tempfile": "^3.0.0",
"tempy": "^0.6.0",
"tsd": "^0.11.0",
"xo": "^0.25.3"
}
Expand Down
71 changes: 35 additions & 36 deletions test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,39 +1,42 @@
import path from 'path';
import fs from 'fs';
import crypto from 'crypto';
import rimraf from 'rimraf';
import test from 'ava';
import tempfile from 'tempfile';
import CpyError from './cpy-error';
import cpy from '.';
const path = require('path');
const fs = require('fs');
const crypto = require('crypto');
const rimraf = require('rimraf');
const test = require('ava');
const tempy = require('tempy');
const proxyquire = require('proxyquire');
const CpyError = require('./cpy-error');
const cpy = require('.');

const read = (...args) => fs.readFileSync(path.join(...args), 'utf8');
const cpyMockedError = module => {
return proxyquire('.', {
[module]() {
throw new Error(`${module}:\tERROR`);
}
});
};

test.beforeEach(t => {
t.context.tmp = tempfile();
t.context.EPERM = tempfile('EPERM');
fs.mkdirSync(t.context.EPERM, 0);
t.context.tmp = tempy.file();
t.context.dir = tempy.directory();
});

test.afterEach(t => {
rimraf.sync(t.context.tmp);
rimraf.sync(t.context.EPERM);
rimraf.sync(t.context.dir);
});

test('reject Errors on missing `source`', async t => {
const error1 = await t.throwsAsync(cpy, /`source`/);
t.true(error1 instanceof CpyError);
await t.throwsAsync(cpy, {message: /`source`/, instanceOf: CpyError});

const error2 = await t.throwsAsync(cpy(null, 'destination'), /`source`/);
t.true(error2 instanceof CpyError);
await t.throwsAsync(cpy(null, 'destination'), {message: /`source`/, instanceOf: CpyError});

const error3 = await t.throwsAsync(cpy([], 'destination'), /`source`/);
t.true(error3 instanceof CpyError);
await t.throwsAsync(cpy([], 'destination'), {message: /`source`/, instanceOf: CpyError});
});

test('reject Errors on missing `destination`', async t => {
const error = await t.throwsAsync(cpy('TARGET'), /`destination`/);
t.true(error instanceof CpyError);
await t.throwsAsync(cpy('TARGET'), {message: /`destination`/, instanceOf: CpyError});
});

test('copy single file', async t => {
Expand All @@ -57,21 +60,21 @@ test('throws on invalid concurrency value', async t => {
test('copy array of files with filter', async t => {
await cpy(['license', 'package.json'], t.context.tmp, {
filter: file => {
if (file.path.endsWith('/license')) {
if (file.path.endsWith(`${path.sep}license`)) {
t.is(file.path, path.join(process.cwd(), 'license'));
t.is(file.relativePath, 'license');
t.is(file.name, 'license');
t.is(file.nameWithoutExtension, 'license');
t.is(file.extension, '');
} else if (file.path.endsWith('/package.json')) {
} else if (file.path.endsWith(`${path.sep}package.json`)) {
t.is(file.path, path.join(process.cwd(), 'package.json'));
t.is(file.relativePath, 'package.json');
t.is(file.name, 'package.json');
t.is(file.nameWithoutExtension, 'package');
t.is(file.extension, 'json');
}

return !file.path.endsWith('/license');
return !file.path.endsWith(`${path.sep}license`);
}
});

Expand All @@ -82,21 +85,21 @@ test('copy array of files with filter', async t => {
test('copy array of files with async filter', async t => {
await cpy(['license', 'package.json'], t.context.tmp, {
filter: async file => {
if (file.path.endsWith('/license')) {
if (file.path.endsWith(`${path.sep}license`)) {
t.is(file.path, path.join(process.cwd(), 'license'));
t.is(file.relativePath, 'license');
t.is(file.name, 'license');
t.is(file.nameWithoutExtension, 'license');
t.is(file.extension, '');
} else if (file.path.endsWith('/package.json')) {
} else if (file.path.endsWith(`${path.sep}package.json`)) {
t.is(file.path, path.join(process.cwd(), 'package.json'));
t.is(file.relativePath, 'package.json');
t.is(file.name, 'package.json');
t.is(file.nameWithoutExtension, 'package');
t.is(file.extension, 'json');
}

return !file.path.endsWith('/license');
return !file.path.endsWith(`${path.sep}license`);
}
});

Expand Down Expand Up @@ -140,7 +143,8 @@ test('path structure', async t => {

await cpy([path.join(t.context.tmp, 'cwd/hello.js')], t.context.tmp, {parents: true});

t.is(read(t.context.tmp, 'cwd/hello.js'), read(t.context.tmp, t.context.tmp, 'cwd/hello.js'));
const {root} = path.parse(t.context.tmp);
t.is(read(t.context.tmp, 'cwd/hello.js'), read(t.context.tmp, t.context.tmp.replace(root, path.sep), 'cwd/hello.js'));
});

test('rename filenames but not filepaths', async t => {
Expand Down Expand Up @@ -175,19 +179,14 @@ test('rename filenames using a function', async t => {
t.is(read(t.context.tmp, 'source/bar.js'), read(t.context.tmp, 'destination/subdir/source/prefix-bar.js'));
});

test('cp-file errors are not glob errors', async t => {
const error = await t.throwsAsync(cpy('license', t.context.EPERM), /EPERM/);
t.notRegex(error.message, /glob/);
});

test('cp-file errors are CpyErrors', async t => {
const error = await t.throwsAsync(cpy('license', t.context.EPERM), /EPERM/);
t.true(error instanceof CpyError);
const cpy = cpyMockedError('cp-file');
await t.throwsAsync(cpy('license', t.context.dir), {message: /cp-file/, instanceOf: CpyError});
});

Copy link
Contributor Author
@brandon93s brandon93s Sep 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

These tests are checking for proper handling of exceptions thrown by dependencies. Previously, the test were implemented with the contrived scenario of a no-access directory which would force these modules to fail: fs.mkdirSync(t.context.EPERM, 0); on *nix systems. This has been replaced with mocking of the dependencies to fail immediately. With this, the additional cp-file + glob test also becomes redundant.

While a similarly contrived example could have been created for Windows ( e.g. writing to a known folder ) successful test execution would have been dependent on the environment the test was run in and whether the shell had elevated privileges.

test('glob errors are CpyErrors', async t => {
const error = await t.throwsAsync(cpy(t.context.EPERM + '/**', t.context.tmp), /EPERM/);
t.true(error instanceof CpyError);
const cpy = cpyMockedError('globby');
await t.throwsAsync(cpy(path.join(t.context.dir, '/**'), t.context.tmp), {message: /globby/, instanceOf: CpyError});
});

test('throws on non-existing file', async t => {
Expand Down
0