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

Duplicate resolution of promises within util #81

Open
matmar10 opened this issue Apr 5, 2020 · 2 comments
Open

Duplicate resolution of promises within util #81

matmar10 opened this issue Apr 5, 2020 · 2 comments

Comments

@matmar10
Copy link

matmar10 commented Apr 5, 2020

Several areas of the lib/utils.js have duplicate resolution of promises - where an error calls BOTH reject AND resolve:

Example:

  return new Promise((resolve, reject) => {
    fs.readFile(file, 'utf8', function(err, data) {
      if (err) {
        reject(err);
      }

      resolve(data);
    });
  });

This will BOTH reject and resolve the error.

Also, the test cases are ambiguously written. It's not clear what behavior is actually expected:

    it('should handle glob error', function(done) {
      var files = new Error('custom: glob error');

      utils.readFileGlobs(files)
      .then(function() {
        done('readFileGlobs did not throw error');
      })
      .catch(function(err) {
        expect(err.message).to.equal('custom: glob error');
        done();
      });
    });

Because the done() method accepts an error. Fro the Mocha docs:

This callback accepts both an Error instance (or subclass thereof) or a falsy value; anything else is invalid usage and throws an error (usually causing a failed test).
matmar10 added a commit to matmar10/livingcss that referenced this issue Apr 5, 2020
@matmar10
Copy link
Author

matmar10 commented Apr 5, 2020

@straker If you can comment on the desired functionality and what the test case means to assert, I can fix this. See PR #82 which already fixes based on what is an assumed reasonable functionality.

@straker
Copy link
Owner

straker commented Apr 5, 2020

Ya for the longest time I just saw any string passed to done() threw the error with the string as the message, so never bothered to turn it into an actual Error. So any time a done() is called with a string that should be a failed test case and should technically be done(new Error(msg)).

For the code, it should probably be return reject(data) and I forgot the return.

matmar10 added a commit to matmar10/livingcss that referenced this issue Aug 30, 2021
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