fix: convert error output to be consistent cross-platform (shelljs#684)

nfischer · freitagbr · commit b6b2cd84ef7e · 2017-03-06T00:54:26.000-08:00
* fix: convert error output to be consistent cross-platform The error output produced by `shell.error()` or `result.stderr` should not be inconsistent between platforms. This ensures that path separators are always printed by ShellJS as `/` instead of as `\` on Windows. This should allow scripts using ShellJS to be more consistent cross-platform. We were not previously relying on error output to always be consistent-- only checking its truthiness. Since this was not part of our tested API, it should be reasonable to change this and release in a patch. Fixes shelljs#681 * Fix broken pushd test case * Fix TODO in a test case
diff --git a/src/common.js b/src/common.js
@@ -80,6 +80,17 @@ function log() {
 }
 exports.log = log;
 
+// Converts strings to be equivalent across all platforms. Primarily responsible
+// for making sure we use '/' instead of '\' as path separators, but this may be
+// expanded in the future if necessary
+function convertErrorOutput(msg) {
+  if (typeof msg !== 'string') {
+    throw new TypeError('input must be a string');
+  }
+  return msg.replace(/\\/g, '/');
+}
+exports.convertErrorOutput = convertErrorOutput;
+
 // Shows error message. Throws if config.fatal is true
 function error(msg, _code, options) {
   // Validate input
@@ -105,7 +116,7 @@ function error(msg, _code, options) {
 
   if (!state.errorCode) state.errorCode = options.code;
 
-  var logEntry = options.prefix + msg;
+  var logEntry = convertErrorOutput(options.prefix + msg);
   state.error = state.error ? state.error + '\n' : '';
   state.error += logEntry;
 
diff --git a/test/common.js b/test/common.js
@@ -47,10 +47,50 @@ test('parseOptions (invalid type)', t => {
   });
 });
 
+test('convertErrorOutput: no args', t => {
+  t.throws(() => {
+    common.convertErrorOutput();
+  }, TypeError);
+});
+
+test('convertErrorOutput: input must be a vanilla string', t => {
+  t.throws(() => {
+    common.convertErrorOutput(3);
+  }, TypeError);
+
+  t.throws(() => {
+    common.convertErrorOutput({});
+  }, TypeError);
+});
+
 //
 // Valids
 //
 
+//
+// common.convertErrorOutput()
+//
+test('convertErrorOutput: nothing to convert', t => {
+  const input = 'hello world';
+  const result = common.convertErrorOutput(input);
+  t.is(result, input);
+});
+
+test('convertErrorOutput: does not change forward slash', t => {
+  const input = 'dir/sub/file.txt';
+  const result = common.convertErrorOutput(input);
+  t.is(result, input);
+});
+
+test('convertErrorOutput: changes backslashes to forward slashes', t => {
+  const input = 'dir\\sub\\file.txt';
+  const result = common.convertErrorOutput(input);
+  t.is(result, 'dir/sub/file.txt');
+});
+
+//
+// common.expand()
+//
 test('single file, array syntax', t => {
   const result = common.expand(['resources/file1.txt']);
   t.falsy(shell.error());
diff --git a/test/mv.js b/test/mv.js
@@ -96,8 +96,7 @@ test('-n option with a directory as the destination', t => {
   const result = shell.mv('-n', 'file1', 'cp');
   t.truthy(shell.error());
   t.is(result.code, 1);
-  // TODO(nate): make this an equals comparison once issue #681 is resolved
-  t.regex(result.stderr, /mv: dest file already exists: cp.file1/);
+  t.is(result.stderr, 'mv: dest file already exists: cp/file1');
 });
 
 test('-f is the default behavior', t => {
diff --git a/test/pushd.js b/test/pushd.js
@@ -273,7 +273,8 @@ test('Push invalid directory', t => {
   shell.pushd('does/not/exist');
   t.is(
     shell.error(),
-    'pushd: no such file or directory: ' + path.resolve('.', 'does/not/exist')
+    `pushd: no such file or directory: ${path.resolve('.', 'does/not/exist')
+      .replace(/\\/g, '/')}`
   );
   t.is(process.cwd(), oldCwd);
 });
