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.

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

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

Closed
jamchamb opened this issue Apr 5, 2016 · 6 comments
Closed

Comments

@jamchamb
Copy link

jamchamb commented Apr 5, 2016

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
Copy link
Owner

davglass commented Apr 5, 2016

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
Copy link
Owner

davglass commented Apr 6, 2016

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
Copy link
Owner

davglass commented Apr 6, 2016

Published this in cpr@1.1.1

@jamchamb
Copy link
Author

jamchamb commented Apr 7, 2016

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
Copy link
Owner

davglass commented Apr 7, 2016

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants