-
Notifications
You must be signed in to change notification settings - Fork 27.5k
fix($q): Promises should be Promises/A+ spec compliant #3535
Conversation
Thanks for the PR!
If you need to make changes to your pull request, you can update the commit with Thanks again for your help! |
Just signed the CLA |
looks like we missed I think that running these tests in karma is an overkill as it requires too many conversions and transformations which will be hard to keep up to date over time. what if we ran the tests in node instead? it should be easy to load also, please add the tests (into qSpec.js) for changes you made to q.js |
If you are OK running those tests in Node, then things should be much easier. |
yes. it doesn't make sense to run that test suite in the browser. we have our $q specs to ensure that the implementation is cross-browser compatible. |
Hi James, do you have cycles to finish this PR as Igor suggested? If nothing else we should extract the actual fix, write a $q spec for it and merge that in. |
Yes. |
Thanks! @IgorMinar and I will take care of the fix for the part of the spec that doesn't pass, so can you just focus on adding the A+ spec suite to our build? |
Just so I'm clear, you want a P.R. that will fail the build because I've added tests but not the implementation? |
Yes. We'll merge our change before yours so that master is never broken, but in your PR the tests should fail on the part of the spec you identified earlier. |
Done. #3693 |
Uses the changes from @jamestalmage's fix in angular#3535. (thanks!) Closes angular#3535
The $q promise library is not Promise/A+ compliant.
The spec's test-suite makes heavy use of Mocha's
done
method and would be difficult to port to jasmine. Therefore, I have added a separate karma config that runs mocha.In addition, the spec is written for Node, and makes use of the CommonJS environment. I forked the spec and used component to make it browser friendly.