davglass / cpr

Nodejs cp -R
Other
79 stars 80 forks source link

Callback not fired if we don't have write permissions for the destination #25

Closed jamchamb closed 8 years ago

jamchamb commented 8 years ago

You can observe this by adding the following test to tests/full.js:

    "should fail without write permissions": {
        topic: function() {
            var self = this;
            var mkdirp = require('mkdirp');
            var baddir = path.join(to, 'readonly');
            mkdirp.sync(baddir);
            fs.chmodSync(baddir, '555');
            cpr(from, baddir, function(err,status) {
                self.callback(null, {
                    err: err,
                    status: status
                });
            });
        },
        "should return an error in the callback": function(topic) {
            assert.isUndefined(topic.status);
            assert(topic.err instanceof Error);
            //assert.equal('From should be a file or directory', topic.err.message);
        }
    },

npm test outputs:

✗ Errored » callback not fired
      in should fail without write permissions
      in CPR Tests
      in tests/full.js
✗ Errored » 38 honored ∙ 1 errored ∙ 1 dropped

npm ERR! Test failed.  See above for more details.
davglass commented 8 years ago

I dropped this test in and it failed for me too. I'll add this to the official test suite and work on a fix for it in the morning.

davglass commented 8 years ago

I've added the skipped test while I figure out where in the stack that it's failing: https://github.com/davglass/cpr/blob/master/tests/full.js#L325-L335

davglass commented 8 years ago

Published this in cpr@1.1.1

jamchamb commented 8 years ago

Thanks for the quick response! The fix seems to work when I include cpr@1.1.1 in another project, but I'm getting errors in some of the new unit tests using node 0.10.44 (they work fine in 4.4.2). I see that it passed in Travis, so I'm trying to figure out if this is just on my end.

Here's the output:

$ node --version
v0.10.44
$ npm --version
2.15.0
$ nvm --version
0.31.0
$ npm test
    ...
    error handling
      ✓ should fail on non-existant from dir
      ✓ should fail on non-file
      ✓ should return an error if a directory is to write over an existing file with the same name
      1) should fail without write permissions
    validations
      2) should copy empty directory
      3) should not delete existing folders in out dir
      4) should copy one file
      5) should not copy because file exists
    should work as a standalone bin
      6) "before all" hook

  29 passing (5m)
  6 failing

  1) cpr test suite error handling should fail without write permissions:
     Error: timeout of 55000ms exceeded. Ensure the done() callback is being called in this test.

  2) cpr test suite validations should copy empty directory:
     Error: timeout of 55000ms exceeded. Ensure the done() callback is being called in this test.

  3) cpr test suite validations should not delete existing folders in out dir:
     Error: timeout of 55000ms exceeded. Ensure the done() callback is being called in this test.

  4) cpr test suite validations should copy one file:
     Error: timeout of 55000ms exceeded. Ensure the done() callback is being called in this test.

  5) cpr test suite validations should not copy because file exists:
     Error: timeout of 55000ms exceeded. Ensure the done() callback is being called in this test.

  6) cpr test suite should work as a standalone bin "before all" hook:
     Error: spawn EMFILE
      at errnoException (child_process.js:1011:11)
      at ChildProcess.spawn (child_process.js:958:11)
      at exports.spawn (child_process.js:746:9)
      at Object.exports.execFile (child_process.js:628:15)
      at exports.exec (child_process.js:591:18)
      at Context.<anonymous> (tests/full.js:376:13)
davglass commented 8 years ago

Hmmm, the timeout issue is likely just slowness from the machine it's running on. Can you try bumping up the timeout to see if that makes them pass? They are copying a massive amount of data. I probably should use a subtree of it so that it's not copying so much data.

As for that last one I'll take a look, my gut tells me it's trying to open the spawn command while the other file operations are still running from the test timeouts, which might show this error.

jamchamb commented 8 years ago

I increased the timeout length to five minutes and it's still failing with the same errors.

I also experimented a bit more with my test project. Trying to copy a small directory with a few files worked fine, but with the node_modules directory for cpr that's used in the unit tests, it times out and doesn't return.

For reference, the machine I'm testing on is running Fedora 23 with an Intel Core 2 Quad Q6600 CPU @ 2.40GHz, 4 GB of RAM, and a 7200 RPM SATA 2.0 disk.