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.

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

Jump to bottom

fix: make test paths Windows safe #87

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: make test paths Windows safe #87

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
42 changes: 30 additions & 12 deletions test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,35 @@ import 'babel-polyfill';

import assert from 'assert';
import { exec } from 'mz/child_process';
import { exists, readFile, writeFile } from 'mz/fs';
import { exists, readFile, writeFile, mkdtemp, mkdir } from 'mz/fs';
Copy link
Member

Choose a reason for hiding this comment

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

Actually, looks like mkdtemp was introduced in node 5, and I'd like to support node 4, so probably best to stick with the old random number approach unless there's a problem with it.

import { join, sep, normalize } from 'path';


import getFilesUnderPath from '../src/util/getFilesUnderPath';

let originalCwd = process.cwd();

async function mkdTempSafe (prefix) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment explaining what safe means here? Looks like it's doing the equivalent of mkdir -p? You could probably also skip this by making sure the tmp-projects folder always exists. I think one way people do that is to commit an empty .gitkeep file in the directory (since git doesn't allow empty tracked directories).

let parts = normalize(prefix).split(sep);
// let last = parts.pop();
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you left this line in by mistake?

let head = '';
for (let part of parts) {
try {
head = join(head, part);
await mkdir(head);
} catch (e) {
if (e.code === 'EEXIST') continue;
throw e;
}
}
return await mkdtemp(prefix);
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I hadn't seen mkdtemp. Does this fix anything for Windows, or is it just a more robust alternative to my manual random directory thing?

}

async function runCli(args) {
let [stdout, stderr] = (await exec(`"${originalCwd}/bin/bulk-decaffeinate" \
--decaffeinate-path "${originalCwd}/node_modules/.bin/decaffeinate" \
--jscodeshift-path "${originalCwd}/node_modules/.bin/jscodeshift" \
--eslint-path "${originalCwd}/node_modules/.bin/eslint" \
let [stdout, stderr] = (await exec(`node "${join(originalCwd,'bin','bulk-decaffeinate')}" \
--decaffeinate-path "${join(originalCwd, 'node_modules', '.bin', 'decaffeinate')}" \
--jscodeshift-path "${join(originalCwd, 'node_modules', '.bin', 'jscodeshift')}" \
--eslint-path "${join(originalCwd, 'node_modules', '.bin', 'eslint')}" \
${args}`));
return {stdout, stderr};
}
Expand Down Expand Up @@ -52,10 +70,10 @@ async function assertFilesEqual(actualFile, expectedFile) {
* given example.
*/
async function runWithTemplateDir(exampleName, fn) {
let suffix = Math.floor(Math.random() * 1000000000000);
let newDir = `./test/tmp-projects/${exampleName}-${suffix}`;
let newDirPref = `./test/tmp-projects/${exampleName}/tmp-`;
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the whole word "prefix" instead of "pref"? In my mind, "pref" means "preference".

Also, looks like you added an extra level of nesting here, which is causing the test failure. I'd rather just keep the same nesting level as before (which also make makes it easier to potentially avoid mkdTempSafe), but I don't have a strong preference.

let newDir;
try {
await exec(`mkdir -p "${newDir}"`);
newDir = await mkdTempSafe(newDirPref);
await exec(`cp -r "./test/examples/${exampleName}/." "${newDir}"`);
process.chdir(newDir);
await fn();
Expand Down Expand Up @@ -109,19 +127,19 @@ describe('simple-error', () => {

await assertFileIncludes(
'decaffeinate-errors.log',
'===== test/examples/simple-error/error.coffee'
`===== ${join('test','examples','simple-error','error.coffee')}`
);

let results = JSON.parse((await readFile('decaffeinate-results.json')).toString());
assert.equal(results.length, 2);
assert.equal(results[0].path, 'test/examples/simple-error/error.coffee');
assert.equal(results[0].path, join('test', 'examples', 'simple-error', 'error.coffee'));
assert.notEqual(results[0].error, null);
assert.equal(results[1].path, 'test/examples/simple-error/success.coffee');
assert.equal(results[1].path, join('test', 'examples', 'simple-error', 'success.coffee'));
assert.equal(results[1].error, null);

await assertFileContents(
'decaffeinate-successful-files.txt',
'test/examples/simple-error/success.coffee'
`${join('test', 'examples', 'simple-error', 'success.coffee')}`
);
});
});
Expand Down