Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(build): Add Promises/A+ Test Suite to the build #3693

Closed

Conversation

jamestalmage
Copy link
Contributor

No description provided.

@IgorMinar
Copy link
Contributor

Thanks! I know that you previously said that you signed our CLA, but I don't see you in our DB. Did you use the same name/email as on github? did you sign as individual or corporation?

if you signed as individual would you might signing again please with the same email address as the one associated with your commits? thanks!


For individuals (a simple click-through form): http://code.google.com/legal/individual-cla-v1.0.html
For corporations (print, sign and scan+email, fax or mail): http://code.google.com/legal/corporate-cla-v1.0.html

@jamestalmage
Copy link
Contributor Author

Done.

@IgorMinar
Copy link
Contributor

this PR still fails even after rebasing on top of the current master which contains 7d188d6 and exposing isFunction to the adapter and replacing null with a noop exceptionHandler implementation.

any idea why?

@IgorMinar
Copy link
Contributor

you can see my current patch above

@jamestalmage
Copy link
Contributor Author

looking now

@jamestalmage
Copy link
Contributor Author

Hmmm. Multiple runs are giving non-deterministic behavior:

I usually only get a single failure (the first), but occasionally the second one pops up.

2 failing

  1) 3.2.5: `then` may be called multiple times on the same promise. 3.2.5.2: If/when `promise` is rejected, respective `onRejected` callbacks must execute in the order of their originating calls to `then`. multiple rejection handlers, one of which throws already-rejected:
     Uncaught 


  2) 3.2.5: `then` may be called multiple times on the same promise. 3.2.5.2: If/when `promise` is rejected, respective `onRejected` callbacks must execute in the order of their originating calls to `then`. results in multiple branching chains with their own fulfillment values already-rejected:
     Uncaught 

I've got to figure out how to get more meaningful stack traces out of the test suite, but the non-determinism is troubling.

@jamestalmage
Copy link
Contributor Author

Link to Failing Test

@jamestalmage
Copy link
Contributor Author

Igor,
I figured it out.
You somehow don't have the patch on q.js.
No isFunction calls to be found anywhere in your branch (in q.js)!

@jamestalmage
Copy link
Contributor Author

Also,
Better console output from the tests can be had if you modify the grunt task as follows:

    shell:{
        "test-promise-a-plus":{
            options:{
                stdout:true,
                stderr:true,
                failOnError:true
            },
            command:'./node_modules/.bin/promises-aplus-tests build/promise-adapter.js --reporter dot'
        }
    },

(changes are: Set stdout to true, and add --reporter dot flag to shell script)

@IgorMinar
Copy link
Contributor

sorry.. I forgot that I removed Brian's/your's commit before pushing to my branch. Even with the commit in place the tests still fail due to one test:

  1) 3.2.5: `then` may be called multiple times on the same promise. 3.2.5.2: If/when `promise` is rejected, respective `onRejected` callbacks must execute in the order of their originating calls to `then`. multiple rejection handlers, one of which throws already-rejected:
     Uncaught 

I don't know what the Uncaught mean here but I'm suspecting that some exception doesn't get handled properly somewhere. I'm looking into this now

@IgorMinar
Copy link
Contributor

41276e5 is my current state with the one failing test

@jamestalmage
Copy link
Contributor Author

This should fix it.

https://github.com/jamestalmage/angular.js/compare/patch-1

I suspect the ref implementation should also wrap the callback with try / catch - but tests are passing with only this change.

@IgorMinar
Copy link
Contributor

I just came to the same conclusion.

Is it just me or is sinon/a+ test suite unusually hard to debug? similar issue in our test suite would be much easier to debug and fix.

@jamestalmage
Copy link
Contributor Author

I don't think the problem is with sinon (I use it regularly, and don't run into this). I'd suspect it's the a+ suite, I found it hard to follow when I was doing my initial patch. Before the switch to running these tests in Node, I was getting far better stack traces. It may be that these tests are doing async processing with nextTick that's making things difficult. That angular allows you to compress your async operations into a synchronous one via a call to $digest, that goes a long ways towards making debugging easier. Kudos to whoever came up with the evalAsync stack. It's pretty brilliant.

@jamestalmage
Copy link
Contributor Author

Oh - I should also mention.
When I made the changes to the shell config above. I went from 1 to 3 failing tests (all related to the same patch we came up with). I think it was related to the stdout:true part, but I don't understand why. Either way, the test output is better with those changes. I missed the part in the promises-aplus docs that showed how to set the reporter from the command line, and it defaults to spec, and I was pretty sure you didn't want all that dumping into the build output, so I only enabled stderr.

@IgorMinar
Copy link
Contributor

ok, I sent out #3699 (based on this PR) for review, please check it out. Thanks a lot for all of your help. I'm closing this PR since #3699 contains your changes and more.

@IgorMinar IgorMinar closed this Aug 22, 2013
IgorMinar pushed a commit to IgorMinar/angular.js that referenced this pull request Aug 22, 2013
IgorMinar pushed a commit that referenced this pull request Aug 24, 2013
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Sep 25, 2013
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
@jamestalmage jamestalmage deleted the promises-aplus-fix-2 branch March 19, 2015 00:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants