Make calls to phantomjs sequential. #156

vinvol · 2015-04-06T23:33:23Z

Hey !

I wrote some changes to make multiple calls to phantomjs sequential.
I'm not sure it's the right approach since I reuse the promise object received from phantomjs but I thought it's worth a shot.

Fixes:
#147

coveralls · 2015-04-06T23:35:43Z

Coverage increased (+0.11%) to 94.47% when pulling 33e6b2d on vinvol:master into fa9e4b0 on stefanjudis:master.

stefanjudis · 2015-04-07T04:36:25Z

Hey @vinvol,

thanks for doing so - but I don't think it's working. :(

Just had a quick look and got:

stefan @ stefan-mac: ~/sites/grunt-phantomas pr/156 ヾ(⌐■_■)ノ
> grunt phantomas                                                                                           [6:22:45]
Running "phantomas:grunt" (phantomas) task

PHANTOMAS EXECUTION(S) STARTED.
>> 5 Phantomas execution(s) done -> checking results:
>> SOMETHING WENT WRONG...
>> TypeError: undefined is not a function
>>     at /Users/stefan/Sites/grunt-phantomas/tasks/lib/phantomas.js:365:26
>>     at Function.filter (/Users/stefan/Sites/grunt-phantomas/node_modules/lodash/dist/lodash.js:3150:15)
>>     at null.<anonymous> (/Users/stefan/Sites/grunt-phantomas/tasks/lib/phantomas.js:364:30)
>>     at tryCatch2 (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/util.js:53:21)
>>     at Promise$_resolveFromResolver [as _resolveFromResolver] (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/promise.js:618:13)
>>     at new Promise (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/promise.js:84:37)
>>     at Phantomas.formResult (/Users/stefan/Sites/grunt-phantomas/tasks/lib/phantomas.js:360:10)
>>     at tryCatch1 (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/util.js:45:21)
>>     at Promise$_callHandler [as _callHandler] (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/promise.js:660:13)
>>     at Promise$_settlePromiseFromHandler [as _settlePromiseFromHandler] (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/promise.js:675:18)

Reason for that is - that the formResult function is expecting promises to perform a double check to prove everything went well during phantomas execution.

Additionally the test suite is doing something weird

stefan @ stefan-mac: ~/sites/grunt-phantomas pr/156 ヾ(⌐■_■)ノ
> npm test                                                                                                  [6:17:52]

> [email protected] test /Users/stefan/Sites/grunt-phantomas
> grunt test

Running "clean:tests" (clean) task
>> 0 paths cleaned.

Running "nodeunit:tests" (nodeunit) task
Testing phantomasTest.js.......................Possibly unhandled TypeError: Cannot assign to read only property 'timestamp' of { "test": test" }
    at null.<anonymous> (/Users/stefan/Sites/grunt-phantomas/tasks/lib/phantomas.js:692:24)
    at tryCatch1 (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/util.js:45:21)
    at Promise$_callHandler [as _callHandler] (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/promise.js:660:13)
    at Promise$_settlePromiseFromHandler [as _settlePromiseFromHandler] (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/promise.js:675:18)
    at Promise$_settlePromiseAt [as _settlePromiseAt] (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/promise.js:845:14)
    at Promise$_settlePromises [as _settlePromises] (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/promise.js:988:14)
    at Async$_consumeFunctionBuffer [as _consumeFunctionBuffer] (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/async.js:77:12)
    at Async$consumeFunctionBuffer (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/async.js:40:14)
    at process._tickCallback (node.js:355:11)
...Possibly unhandled TypeError: Cannot assign to read only property 'timestamp' of { "test": "test2",,,,, }
    at null.<anonymous> (/Users/stefan/Sites/grunt-phantomas/tasks/lib/phantomas.js:692:24)
    at tryCatch1 (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/util.js:45:21)
    at Promise$_callHandler [as _callHandler] (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/promise.js:660:13)
    at Promise$_settlePromiseFromHandler [as _settlePromiseFromHandler] (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/promise.js:675:18)
    at Promise$_settlePromiseAt [as _settlePromiseAt] (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/promise.js:845:14)
    at Promise$_settlePromises [as _settlePromises] (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/promise.js:988:14)
    at Async$_consumeFunctionBuffer [as _consumeFunctionBuffer] (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/async.js:77:12)
    at Async$consumeFunctionBuffer (/Users/stefan/Sites/grunt-phantomas/node_modules/bluebird/js/main/async.js:40:14)
    at process._tickCallback (node.js:355:11)
.......OK
>> 124 assertions passed (48714ms)

Running "jshint:all" (jshint) task
>> 4 files lint free.

Running "jscs:src" (jscs) task
>> 4 files without code style errors.

Done, without errors.

But thanks for doing that and I'm happy to support you if help is needed. :)
Wanna give it another shot?

Personally I'd stick with the array of promises and Promise.all, but push anytime the last one is fulfilled/rejected a new one to the end.

Quick tip:

For development you can always run grunt phantomas straight from the repo itself and npm test is giving you a good overview if everything works fine.

coveralls · 2015-04-07T20:57:48Z

Coverage increased (+0.11%) to 94.47% when pulling 05f1a47 on vinvol:master into fa9e4b0 on stefanjudis:master.

Missing comma

coveralls · 2015-04-07T21:08:40Z

Coverage increased (+0.64%) to 95.0% when pulling 5a72168 on vinvol:master into fa9e4b0 on stefanjudis:master.

vinvol · 2015-04-07T21:14:35Z

Hey @stefanjudis,

my fork was a little bit behind, sorry about that :/

The tests should pass now :p

stefanjudis · 2015-04-07T21:28:41Z

Cool cool cool! 👍
Will check it out probably tomorrow.

May i ask for one more thing? 😊

Can you stick to coding style of the project?

It's basically writter with much more spaces everywhere. :D

https://github.com/stefanjudis/grunt-phantomas/blob/master/CONTRIBUTING.md

vinvol · 2015-04-07T22:04:41Z

No problem ;)

coveralls · 2015-04-07T22:07:05Z

Coverage increased (+0.64%) to 95.0% when pulling ca0f96a on vinvol:master into fa9e4b0 on stefanjudis:master.

stefanjudis · 2015-04-08T19:01:01Z

tasks/lib/phantomas.js

- .catch( function( e ) {
- console.log( e );
- } );
+ Promise.reduce( runs, function( total, run, index ) {


Hey hey,

maybe I'm wrong, but an each would do the same trick or?
There is actually no reduction.

https://github.com/petkaantonov/bluebird/blob/master/API.md#eachfunction-iterator---promise

stefanjudis · 2015-04-08T19:25:44Z

@vinvol

Hmm, I played around with it a bit and liked the approach. Unfortunately there is one downside with this.

From the reduce or each docs:

If any promise in the input array is rejected the returned promise is rejected as well.

Current behavior is, that if any phantomas execution fails grunt-phantomas is still able to go on and just use the succeeded ones. As long as there are any. :)

With this approach this is not working anymore, or? Because one failing phantomas call is already leading to complete fail.

Please correct me when I'm wrong.

…ed or failed.

coveralls · 2015-04-09T23:35:32Z

Coverage increased (+0.17%) to 94.53% when pulling da6bc50 on vinvol:master into fa9e4b0 on stefanjudis:master.

vinvol · 2015-04-09T23:35:56Z

@stefanjudis

You were perfectly right!

I replaced the reduce by an .each() and used the reflect() to ensure that the loop would go on, event if one of the call fails.
However, I still keep the result in the array of runs, and settle as in the original code.

😝

stefanjudis · 2015-04-09T23:45:22Z

@vinvol

Nice - on first look this looks great! Will have a more detailed look on the weekend.

Thanks for doing it.

Make calls to phantomjs sequential.

stefanjudis · 2015-04-11T17:31:52Z

Congrats and thanks so much!

@vinvol

Great job!

Make calls to phantomjs sequential.

33e6b2d

Merge remote-tracking branch 'upstream/master'

05f1a47

stefanjudis and others added 5 commits April 7, 2015 17:03

stefanjudis#145 reordering readme

ff69d4f

add limitIncludedRuns - fix stefanjudis#145

04ad31c

Missing comma in gruntfile example - README.md

fdac05d

Missing comma

Make calls to phantomas sequential

544317a

Merge branch 'master' of https://github.com/vinvol/grunt-phantomas

5a72168

Code style

ca0f96a

stefanjudis reviewed Apr 8, 2015
View reviewed changes

Make calls to pahntomas sequential. Keep results of each run, succeed…

da6bc50

…ed or failed.

stefanjudis added a commit that referenced this pull request Apr 11, 2015

Merge pull request #156 from vinvol/master

bd8039d

Make calls to phantomjs sequential.

stefanjudis merged commit bd8039d into stefanjudis:master Apr 11, 2015

stefanjudis added this to the v0.12.0 milestone Apr 11, 2015

stefanjudis self-assigned this Apr 11, 2015

stefanjudis mentioned this pull request Apr 11, 2015

Phantomas runs are not synchronous #147

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make calls to phantomjs sequential. #156

Make calls to phantomjs sequential. #156

vinvol commented Apr 6, 2015

coveralls commented Apr 6, 2015

stefanjudis commented Apr 7, 2015

coveralls commented Apr 7, 2015

coveralls commented Apr 7, 2015

vinvol commented Apr 7, 2015

stefanjudis commented Apr 7, 2015

vinvol commented Apr 7, 2015

coveralls commented Apr 7, 2015

stefanjudis Apr 8, 2015

stefanjudis commented Apr 8, 2015

coveralls commented Apr 9, 2015

vinvol commented Apr 9, 2015

stefanjudis commented Apr 9, 2015

stefanjudis commented Apr 11, 2015

Make calls to phantomjs sequential. #156

Make calls to phantomjs sequential. #156

Conversation

vinvol commented Apr 6, 2015

coveralls commented Apr 6, 2015

stefanjudis commented Apr 7, 2015

coveralls commented Apr 7, 2015

coveralls commented Apr 7, 2015

vinvol commented Apr 7, 2015

stefanjudis commented Apr 7, 2015

vinvol commented Apr 7, 2015

coveralls commented Apr 7, 2015

stefanjudis Apr 8, 2015

Choose a reason for hiding this comment

stefanjudis commented Apr 8, 2015

coveralls commented Apr 9, 2015

vinvol commented Apr 9, 2015

stefanjudis commented Apr 9, 2015

stefanjudis commented Apr 11, 2015