promises-aplus / promises-tests

Compliances tests for Promises/A+
Other
943 stars 107 forks source link

Ractive.Promise pass all tests, but behaves incorrectly. #60

Closed svbatalov closed 10 years ago

svbatalov commented 10 years ago

Ractive.Promise promises implementation pass all tests, but fails to convert exceptions to rejections. Here is the test code:

var promisesAplusTests = require("promises-aplus-tests");
var Promise = require('ractive').Promise;
//var Promise = require('promise');
//var Promise = require('q').Promise;   // get stuck at tests
//var Promise = require('when').Promise;

var adapter = {
    resolved: Promise.resolve,
    rejected: Promise.reject,
    deferred: function() {
        var res, rej;
        var p = new Promise(function (resolve, reject) {
            res = resolve;
            rej = reject;
        });
        return {
            promise: p,
            resolve: res,
            reject: rej
        };
    }
};

var throw_test = function () {
    var p = new Promise(function (resolve, reject) {
        throw new Error('Bad happens');
    });
    return p;
};

if(0) promisesAplusTests(adapter, function (err) {
    if(err) console.log('Errors:', err);
});

if(1) throw_test().then(undefined, function (err) {
    console.log('Error in promise:', err);
});

Other three implementations (commented-out in code above) reject the promise, which I believe is correct.

So, should Ractive.Promise fail compliance test?

briancavalier commented 10 years ago

@svbatalov Technically, there is no Promises/A+ specification for the Promise constructor. These tests are designed only to test then()'s compliance with http://promisesaplus.com. So, even though the test suite does require some way of creating promises in order to test then, I think this particular case is outside the scope of the tests. IOW, since your implementation passes the tests, it is Promises/A+ compliant :)

In practice, as you observed, most libraries that implement a Promise constructor do catch those exceptions and reject the newly constructed promise. The ES6 Promise spec requires that as well. So I'd def say, though, that adding a try/catch and rejecting would be the way to go.

svbatalov commented 10 years ago

Thanks!

Rich-Harris commented 10 years ago

Thanks a lot for the clarification @briancavalier - we've fixed it

briancavalier commented 10 years ago

@Rich-Harris Cool, and no problem at all.