Refactor to fix bugs and update unit tests

src/shell/shell.js

@@ -4,7 +4,7 @@ var Environment = require('./environment.js');
 var async = require('../../lib/async.js');
 var Encoding = require('../encoding.js');
 var minimatch = require('minimatch');
-var Constants = require('src/constants');
+var ROOT_DIRECTORY_NAME = require('../constants').ROOT_DIRECTORY_NAME;
 
 function Shell(fs, options) {
   options = options || {};
@@ -429,9 +429,9 @@ Shell.prototype.mkdirp = function(path, callback) {
 };
 
 /**
- * Moves the file or directory at the `source` path to the
+ * Renames or moves `source` to `destination`, or moves each path
- * `destination` path by relinking the source to the destination
+ * in the `source` array into `destination`. Overwrites files in
- * path.
+ * `destination` that have the same name(s) as `source` by default.
  */
 Shell.prototype.mv = function(source, destination, callback) {
   var fs = this.fs;
@@ -440,16 +440,17 @@ Shell.prototype.mv = function(source, destination, callback) {
   callback = callback || function() {};
 
   if(!source) {
-    callback(new Errors.EINVAL('missing source path argument'));
+    callback(new Errors.EINVAL('Missing source path argument'));
     return;
   }
-  else if(source === Constants.ROOT_DIRECTORY_NAME) {
+
-    callback(new Errors.EINVAL('the root directory is not a valid source argument'));
+  if(source === ROOT_DIRECTORY_NAME) {
+    callback(new Errors.EINVAL('The root directory is not a valid source argument'));
     return;
   }
 
   if(!destination) {
-    callback(new Errors.EINVAL('missing destination path argument'));
+    callback(new Errors.EINVAL('Missing destination path argument'));
     return;
   }
 
@@ -458,13 +459,6 @@ Shell.prototype.mv = function(source, destination, callback) {
     destpath = Path.resolve(this.cwd, destpath);
     var destdir = Path.dirname(destpath);
 
-    // Recursively create any directories on the destination path which do not exist
-    shell.mkdirp(destdir, function(error) {
-      if(error) {
-        callback(error);
-        return;
-      }
-
     // If there is no node at the source path, error and quit
     fs.lstat(sourcepath, function(error, sourcestats) {
       if(error) {
@@ -472,7 +466,15 @@ Shell.prototype.mv = function(source, destination, callback) {
         return;
       }
 
-        fs.lstat(destpath, function(error, deststats) {
+      // If the destination's parent directory doesn't exist, error and quit
+      fs.lstat(destdir, function(error) {
+        // If there is an error unrelated to the existence of the destination, exit
+        if(error) {
+          callback(error);
+          return;
+        }
+
+        fs.lstat(destpath, function(error, deststats){
           // If there is an error unrelated to the existence of the destination, exit
           if(error && error.code !== 'ENOENT') {
             callback(error);
@@ -480,9 +482,13 @@ Shell.prototype.mv = function(source, destination, callback) {
           }
 
           if(deststats) {
+            if (deststats.isDirectory()) {
               // If the destination is a directory, new destination is destpath/source.basename
-            if(deststats.isDirectory()) {
               destpath = Path.join(destpath, Path.basename(sourcepath));
+            } else if(sourcestats.isDirectory()) {
+              // If the source is a folder, but the destination isn't, error and exit
+              callback(new Errors.EINVAL("Destination is not a directory"));
+              return;
             }
           }
 
@@ -492,14 +498,15 @@ Shell.prototype.mv = function(source, destination, callback) {
               callback(error);
               return;
             }
-            // If the source is a file, link it to destination and remove the source, then done
+
+            // If the source isn't a directory, link it to destination and remove the source, then done
             if(sourcestats.isFile() || sourcestats.isSymbolicLink()) {
               fs.link(sourcepath, destpath, function(error) {
                 if (error) {
                   callback(error);
                   return;
                 }
-                shell.rm(sourcepath, {recursive:true}, function(error) {
+                fs.unlink(sourcepath, function(error) {
                   if (error) {
                     callback(error);
                     return;
@@ -507,10 +514,12 @@ Shell.prototype.mv = function(source, destination, callback) {
                   callback();
                 });
               });
+              return;
             }
+
             // If the source is a directory, create a directory at destination and then recursively
             // move every dir entry.
-            else if(sourcestats.isDirectory()) {
+            if(sourcestats.isDirectory()) {
               fs.mkdir(destpath, function(error) {
                 if (error) {
                   callback(error);
@@ -523,8 +532,8 @@ Shell.prototype.mv = function(source, destination, callback) {
                     return;
                   }
 
-                  // Asychronously applies move to all nodes in the source directory
+                  // Apply the move to all nodes in the directory
-                  async.each(entries,
+                  async.eachSeries(entries,
                     function(entry, callback) {
                       move(Path.join(sourcepath, entry), Path.join(destpath, entry), function(error) {
                         if(error) {

tests/spec/shell/mv.spec.js

@@ -1,6 +1,8 @@
-define(["Filer", "util"], function(Filer, util) {
+var Filer = require('../../..');
+var util = require('../../lib/test-utils.js');
+var expect = require('chai').expect;
 
-  describe('FileSystemShell.mv', function() {
+describe('FileSystemShell.mv', function() {
   beforeEach(util.setup);
   afterEach(util.cleanup);
 
@@ -11,7 +13,7 @@ define(["Filer", "util"], function(Filer, util) {
 
   it('should fail when source argument is absent', function(done) {
     var fs = util.fs();
-      var shell = fs.Shell();
+    var shell = new fs.Shell();
 
     shell.mv(null, null, function(error) {
       expect(error).to.exist;
@@ -21,7 +23,7 @@ define(["Filer", "util"], function(Filer, util) {
 
   it('should fail when destination argument is absent', function(done) {
     var fs = util.fs();
-      var shell = fs.Shell();
+    var shell = new fs.Shell();
     var contents = "a";
 
     fs.writeFile('/file', contents, function(error) {
@@ -34,62 +36,94 @@ define(["Filer", "util"], function(Filer, util) {
     });
   });
 
-    it('should fail when arguments are empty strings', function(done) {
+  it('should fail when source argument is an empty string', function(done) {
     var fs = util.fs();
-      var shell = fs.Shell();
+    var shell = new fs.Shell();
 
-      shell.mv('', '', function(error) {
+    shell.mv('', '/file', function(error) {
       expect(error).to.exist;
       done();
     });
   });
 
+  it('should fail when destination argument is an empty string', function(done) {
+    var fs = util.fs();
+    var shell = new fs.Shell();
+    var contents = 'a';
+
+    fs.writeFile('/file', contents, function(error) {
+      if(error) throw error;
+      shell.mv('/file', '', function(error) {
+        expect(error).to.exist;
+        done();
+      });
+    });
+  });
+
   it('should fail when the node at source path does not exist', function(done) {
     var fs = util.fs();
-      var shell = fs.Shell();
+    var shell = new fs.Shell();
 
     fs.mkdir('/dir', function(error) {
-        expect(error).to.not.exist;
+      if(error) throw error;
-      });
-
       shell.mv('/file', '/dir', function(error) {
         expect(error).to.exist;
         done();
       });
     });
+  });
 
   it('should fail when root is provided as source argument', function(done) {
     var fs = util.fs();
-      var shell = fs.Shell();
+    var shell = new fs.Shell();
 
     fs.mkdir('/dir', function(error) {
-        expect(error).to.not.exist;
+      if(error) throw error;
-      });
-
       shell.mv('/', '/dir', function(error) {
         expect(error).to.exist;
         done();
       });
     });
+  });
+
+  it('should fail when node at source path is a folder, but node at destination is a file', function(done) {
+    var fs = util.fs();
+    var shell = new fs.Shell();
+    var contents = "a";
+
+    fs.mkdir ('/dir', function(error) {
+      if(error) throw error;
+
+      fs.writeFile('/file', contents, function(error) {
+        if(error) throw error;
+
+        shell.mv('/dir', '/file', function(error) {
+          expect(error).to.exist;
+          expect(error.code).to.equal('EINVAL');
+          done();
+        });
+      });
+    });
+  });
 
   it('should rename a file which is moved to the same directory under a different name', function(done) {
     var fs = util.fs();
-      var shell = fs.Shell();
+    var shell = new fs.Shell();
     var contents = "a";
 
     fs.writeFile('/file', contents, function(error) {
-        expect(error).to.not.exist;
+      if(error) throw error;
 
       shell.mv('/file', '/newfile', function(error) {
-          expect(error).to.not.exist;
+        if(error) throw error;
 
         fs.stat('/file', function(error, stats) {
           expect(error).to.exist;
-            expect(stats).to.not.exist;
+          expect(stats).not.to.exist;
 
-            fs.stat('/newfile', function(error, stats) {
+          fs.readFile('/newfile', 'utf8', function(error, data) {
-              expect(error).to.not.exist;
+            expect(error).not.to.exist;
-              expect(stats).to.exist;
+            expect(data).to.equal(contents);
             done();
           });
         });
@@ -99,29 +133,27 @@ define(["Filer", "util"], function(Filer, util) {
 
   it('should rename a symlink which is moved to the same directory under a different name', function(done) {
     var fs = util.fs();
-      var shell = fs.Shell();
+    var shell = new fs.Shell();
     var contents = "a";
 
     fs.writeFile('/file', contents, function(error) {
-        expect(error).to.not.exist;
+      if(error) throw error;
 
       fs.symlink('/file', '/newfile', function(error) {
-          expect(error).to.not.exist;
+        if(error) throw error;
 
         shell.mv('/newfile', '/newerfile', function(error) {
-            expect(error).to.not.exist;
+          expect(error).not.to.exist;
 
           fs.stat('/file', function(error, stats) {
-              expect(error).to.not.exist;
+            expect(error).not.to.exist;
-              expect(stats).to.exist;
 
             fs.stat('/newfile', function(error, stats) {
               expect(error).to.exist;
-                expect(stats).to.not.exist;
 
-                fs.stat('/newerfile', function(error, stats) {
+              fs.readFile('/newerfile', 'utf8', function(error, data) {
-                  expect(error).to.not.exist;
+                expect(error).not.to.exist;
-                  expect(stats).to.exist;
+                expect(data).to.equal(contents);
                 done();
               });
             });
@@ -131,60 +163,27 @@ define(["Filer", "util"], function(Filer, util) {
     });
   });
 
-    it('should move a file to a directory which does not currently exist', function(done) {
-      var fs = util.fs();
-      var shell = fs.Shell();
-      var contents = "a";
-
-      fs.writeFile('/file', contents, function(error) {
-      	expect(error).to.not.exist;
-
-        shell.mv('/file', '/dir/newfile', function(error) {
-          expect(error).to.not.exist;
-          
-          fs.stat('/file', function(error, stats) {
-            expect(error).to.exist;
-            expect(stats).to.not.exist;
-            
-            fs.stat('/dir/newfile', function(error, stats) {
-              expect(error).to.not.exist;
-              expect(stats).to.exist;
-              done();
-            });
-          });
-        });
-      });
-    });
-
   it('should move a file into an empty directory', function(done) {
     var fs = util.fs();
-      var shell = fs.Shell();
+    var shell = new fs.Shell();
     var contents = "a";
 
     fs.mkdir('/dir', function(error) {
-        expect(error).to.not.exist;
+      if(error) throw error;
-        
-        fs.stat('/dir', function(error, stats) {
-          expect(error).to.not.exist;
-          expect(stats).to.exist;
 
       fs.writeFile('/file', contents, function(error) {
-            expect(error).to.not.exist;
+        if(error) throw error;
-            
-            fs.stat('/file', function(error, stats) {
-              expect(error).to.not.exist;
-              expect(stats).to.exist;
 
         shell.mv('/file', '/dir', function(error) {
           expect(error).to.not.exist;
 
           fs.stat('/file', function(error, stats) {
             expect(error).to.exist;
-                  expect(stats).to.not.exist;
+            expect(error.code).to.equal('ENOENT');
 
-                  fs.stat('/dir/file', function(error, stats) {
+            fs.readFile('/dir/file', 'utf8', function(error, data) {
-                    expect(error).to.not.exist;
+              expect(error).not.to.exist;
-                    expect(stats).to.exist;
+              expect(data).to.equal(contents);
               done();
             });
           });
@@ -192,34 +191,28 @@ define(["Filer", "util"], function(Filer, util) {
       });
     });
   });
-      });
-    });
 
   it('should move a file into a directory that has a file of the same name', function(done) {
     var fs = util.fs();
-      var shell = fs.Shell();
+    var shell = new fs.Shell();
     var contents = "a";
     var contents2 = "b";
 
     fs.mkdir('/dir', function(error) {
-        expect(error).to.not.exist;
+      if(error) throw error;
 
       fs.writeFile('/file', contents, function(error) {
-          expect(error).to.not.exist;
+        if(error) throw error;
 
         fs.writeFile('/dir/file', contents2, function(error) {
-            expect(error).to.not.exist;
+          if(error) throw error;
 
           shell.mv('/file', '/dir/file', function(error) {
             expect(error).to.not.exist;
 
             fs.stat('/file', function(error, stats) {
               expect(error).to.exist;
-                expect(stats).to.not.exist;
+              expect(error.code).to.equal('ENOENT');
-                  
-                fs.stat('/dir/file', function(error, stats) {
-                  expect(error).to.not.exist;
-                  expect(stats).to.exist;
 
               fs.readFile('/dir/file', 'utf8', function(error, data) {
                 expect(error).not.to.exist;
@@ -232,48 +225,23 @@ define(["Filer", "util"], function(Filer, util) {
       });
     });
   });
-    });
-
-    it('should move an empty directory to a destination that does not currently exist', function(done) {
-      var fs = util.fs();
-      var shell = fs.Shell();
-      
-      fs.mkdir('/dir', function(error) {
-        expect(error).to.not.exist;
-
-        shell.mv('/dir', '/newdir', function(error) {
-          expect(error).to.not.exist;
-
-          fs.stat('/dir', function(error, stats) {
-            expect(error).to.exist;
-            expect(stats).to.not.exist;
-
-            fs.stat('/newdir', function(error, stats) {
-              expect(error).to.not.exist;
-              expect(stats).to.exist;
-              done();
-            });
-          });
-        });
-      });
-    });
 
   it('should move an empty directory to another empty directory', function(done) {
     var fs = util.fs();
-      var shell = fs.Shell();
+    var shell = new fs.Shell();
 
     fs.mkdir('/dir', function(error) {
-        expect(error).to.not.exist;
+      if(error) throw error;
 
       fs.mkdir('/otherdir', function(error) {
-          expect(error).to.not.exist;
+        if(error) throw error;
 
         shell.mv('/dir', '/otherdir', function(error) {
           expect(error).to.not.exist;
 
           fs.stat('/dir', function(error, stats) {
             expect(error).to.exist;
-              expect(stats).to.not.exist;
+            expect(error.code).to.equal('ENOENT');
 
             fs.stat('/otherdir/dir', function(error, stats) {
               expect(error).to.not.exist;
@@ -288,24 +256,24 @@ define(["Filer", "util"], function(Filer, util) {
 
   it('should move an empty directory to a populated directory', function(done) {
     var fs = util.fs();
-      var shell = fs.Shell();
+    var shell = new fs.Shell();
     var contents = "a";
 
     fs.mkdir('/dir', function(error) {
-        expect(error).to.not.exist;
+      if(error) throw error;
 
       fs.mkdir('/otherdir', function(error) {
-          expect(error).to.not.exist;
+        if(error) throw error;
 
         fs.writeFile('/otherdir/file', contents, function(error) {
-            expect(error).to.not.exist;
+          if(error) throw error;
 
           shell.mv('/dir', '/otherdir', function(error) {
             expect(error).to.not.exist;
 
             fs.stat('/dir', function(error, stats) {
               expect(error).to.exist;
-                expect(stats).to.not.exist;
+              expect(error.code).to.equal('ENOENT');
 
               fs.stat('/otherdir/file', function(error, stats) {
                 expect(error).to.not.exist;
@@ -326,39 +294,40 @@ define(["Filer", "util"], function(Filer, util) {
 
   it('should move a populated directory to a populated directory', function(done) {
     var fs = util.fs();
-      var shell = fs.Shell();
+    var shell = new fs.Shell();
     var contents = "a";
+    var contents2 = "b";
 
     fs.mkdir('/dir', function(error) {
-        expect(error).to.not.exist;
+      if(error) throw error;
 
       fs.mkdir('/otherdir', function(error) {
-          expect(error).to.not.exist;
+        if(error) throw error;
 
         fs.writeFile('/otherdir/file', contents, function(error) {
-            expect(error).to.not.exist;
+          if(error) throw error;
 
-            fs.writeFile('/dir/file', contents, function(error) {
+          fs.writeFile('/dir/file', contents2, function(error) {
-              expect(error).to.not.exist;
+            if(error) throw error;
 
             shell.mv('/dir', '/otherdir', function(error) {
               expect(error).to.not.exist;
 
               fs.stat('/dir', function(error, stats) {
                 expect(error).to.exist;
-                  expect(stats).to.not.exist;
+                expect(error.code).to.equal('ENOENT');
 
-                  fs.stat('/otherdir/file', function(error, stats) {
+                fs.readFile('/otherdir/file', 'utf8', function(error, data) {
                   expect(error).to.not.exist;
-                    expect(stats).to.exist;
+                  expect(data).to.equal(contents);
 
                   fs.stat('/otherdir/dir', function(error, stats) {
                     expect(error).to.not.exist;
                     expect(stats).to.exist;
 
-                      fs.stat('/otherdir/dir/file', function(error, stats) {
+                    fs.readFile('/otherdir/dir/file', 'utf8', function(error, data) {
                       expect(error).to.not.exist;
-                        expect(stats).to.exist;
+                      expect(data).to.equal(contents2);
                       done();
                     })
                   });
@@ -370,119 +339,4 @@ define(["Filer", "util"], function(Filer, util) {
       });
     });
   });
-
-    it('should move an empty directory to another empty directory', function(done) {
-      var fs = util.fs();
-      var shell = fs.Shell();
-      
-      fs.mkdir('/dir', function(error) {
-        expect(error).to.not.exist;
-
-        fs.mkdir('/otherdir', function(error) {
-          expect(error).to.not.exist;
-
-          shell.mv('/dir', '/otherdir', function(error) {
-            expect(error).to.not.exist;
-
-            fs.stat('/dir', function(error, stats) {
-              expect(error).to.exist;
-              expect(stats).to.not.exist;
-
-              fs.stat('/otherdir/dir', function(error, stats) {
-                expect(error).to.not.exist;
-                expect(stats).to.exist;
-                done();
-              });
-            });
-          });
-        });
-      });
-    });
-
-    it('should move an empty directory to a populated directory', function(done) {
-      var fs = util.fs();
-      var shell = fs.Shell();
-      var contents = "a";
-      
-      fs.mkdir('/dir', function(error) {
-        expect(error).to.not.exist;
-
-        fs.mkdir('/otherdir', function(error) {
-          expect(error).to.not.exist;
-
-          fs.writeFile('/otherdir/file', contents, function(error) {
-            expect(error).to.not.exist;
-
-            shell.mv('/dir', '/otherdir', function(error) {
-              expect(error).to.not.exist;
-
-              fs.stat('/dir', function(error, stats) {
-                expect(error).to.exist;
-                expect(stats).to.not.exist;
-
-                fs.stat('/otherdir/file', function(error, stats) {
-                  expect(error).to.not.exist;
-                  expect(stats).to.exist;
-
-                  fs.stat('/otherdir/dir', function(error, stats) {
-                    expect(error).to.not.exist;
-                    expect(stats).to.exist;
-                    done();
-                  });
-                });
-              });
-            });
-          });
-        });
-      });
-    });
-
-    it('should move a populated directory to a populated directory', function(done) {
-      var fs = util.fs();
-      var shell = fs.Shell();
-      var contents = "a";
-      
-      fs.mkdir('/dir', function(error) {
-        expect(error).to.not.exist;
-
-        fs.mkdir('/otherdir', function(error) {
-          expect(error).to.not.exist;
-
-          fs.writeFile('/otherdir/file', contents, function(error) {
-            expect(error).to.not.exist;
-
-            fs.writeFile('/dir/file', contents, function(error) {
-              expect(error).to.not.exist;
-
-              shell.mv('/dir', '/otherdir', function(error) {
-                expect(error).to.not.exist;
-
-                fs.stat('/dir', function(error, stats) {
-                  expect(error).to.exist;
-                  expect(stats).to.not.exist;
-
-                  fs.stat('/otherdir/file', function(error, stats) {
-                    expect(error).to.not.exist;
-                    expect(stats).to.exist;
-
-                    fs.stat('/otherdir/dir', function(error, stats) {
-                      expect(error).to.not.exist;
-                      expect(stats).to.exist;
-
-                      fs.stat('/otherdir/dir/file', function(error, stats) {
-                        expect(error).to.not.exist;
-                        expect(stats).to.exist;
-                        done();
-                      })
-                    });
-                  });
-                });
-              });
-            });
-          });
-        });
-      });
-    });
-  });
 });
-