search

elasticio / sphereio-component

elastic.io component for commercetools
http://elastic.io
Apache License 2.0
10 stars 3 forks source link

Update sample library #63

Closed pvoznenko closed 9 years ago

pvoznenko commented 9 years ago

Hi,

With first commit (be5832b) I removed 'node_modules' folder. Since it is not nice to have external dependencies in main library.

So before running tests in your build tool (I see you using Codeship), please run command:

$ npm install

To install all following dependancies.

Best regards, Pavlo Voznenko

pvoznenko commented 9 years ago

With second commit (c924542) I fixed warnings at npm package file, also added npm test possibility.

Unfortunately there is still warning related to issue #62.

pvoznenko commented 9 years ago

With 3rd commit (edd3c16) I added possibility to build with Travis. Issue #60

Notice that Travis will run command:

$ npm test

to test build. Now it possible after 2nd commit c924542

drobiazko commented 9 years ago

Hey Pavlo,

thank you very much for this pull request. It's awesome to see you contributing. However, we can't remove the node_modules folder as our platform doesn't invoke npm install on production servers yet.

Can you please provide a new pull request without removal of node_modules please? If you are updating the dependencies just commit them along with the the code changes.

pvoznenko commented 9 years ago

Hi Igor,

Ok, no problem, I just saw that in .gitignore file you have folder node_modules ignored :)

One moment, will rebase.

pvoznenko commented 9 years ago

Done.

Now this pull request contain:

Do you need help with Travis?

I also figure out when tried to solve issue #62 , changing library to sphere-node-sdk, that in test with new sdk client for some reason hangs :(

Any ideas why? Otherwise I could look at it a little bit later.

Have nice day!

mmoelli commented 9 years ago

Hey @pvoznenko Thanks for adding this here. Can you provide an error message or give an hint where it hangs. @emmenko could take a quick look.

Bests, Martin

pvoznenko commented 9 years ago

Hi @mmoelli

First of all, I used "sphere-node-sdk": "1.2.0" and in lib -> sphere.js:

var SphereClient = require('sphere-node-sdk').SphereClient;

I debugged a little bit and got following error:

% npm test  

> sphereio-spec@ test /Users/pavlo/Projects/sphereio-api
> grunt test

Running "jasmine_node:spec" (jasmine_node) task
Possibly unhandled ConcurrentModification: Version mismatch. Concurrent modification.
    at /Users/pavlo/Projects/sphereio-api/node_modules/sphere-node-sdk/lib/services/base.js:640:22
    at ProductService.BaseService._wrapResponse (/Users/pavlo/Projects/sphereio-api/node_modules/sphere-node-sdk/lib/services/base.js:651:11)
    at /Users/pavlo/Projects/sphereio-api/node_modules/sphere-node-sdk/lib/services/base.js:542:40
    at Request._callback (/Users/pavlo/Projects/sphereio-api/node_modules/sphere-node-sdk/lib/connect/rest.js:210:14)
    at Request.self.callback (/Users/pavlo/Projects/sphereio-api/node_modules/sphere-node-sdk/node_modules/request/request.js:373:22)
    at Request.emit (events.js:98:17)
    at Request.<anonymous> (/Users/pavlo/Projects/sphereio-api/node_modules/sphere-node-sdk/node_modules/request/request.js:1318:14)
    at Request.emit (events.js:117:20)
    at OutgoingMessage.<anonymous> (/Users/pavlo/Projects/sphereio-api/node_modules/sphere-node-sdk/node_modules/request/request.js:1266:12)
    at OutgoingMessage.emit (events.js:117:20)

Error comes from lib -> sphere.js function createClient on creation of new instance of SphereClient.

So http error 409.

I not really familiar with changes that you did between versions. I believe you could easy see whats missing in spec -> sphere.spec.js file.

Hopefully it is useful information.

Unfortunately did not go deeper yet.

pvoznenko commented 9 years ago

So I found out that behavior of Object BaseService.prototype._wrapResponse changed in sphere-node-sdk compare to sphere-node-client.

So thats why on testing it hangs (cases not covered). In test it expected something different then got back from new library.

pvoznenko commented 9 years ago

I think now I found it, in old version you had method fail for client, in new version it called catch

pvoznenko commented 9 years ago

So with updating fail to catch currently 9 test are failed.

Some errors just because of response signiture was changed like:

Expected spy self.emit to have been called with [ 'error', { statusCode : 404, message : 'Endpoint '/elasticio/customers/54' not found.', originalRequest : { endpoint : '/customers/54' } } ] but actual calls were [ 'error', { body : { statusCode : 404, message : 'Endpoint '/elasticio/customers/54' not found.', originalRequest : { endpoint : '/customers/54' } }, code : 404 } ], [ 'end' ]

Some contain test expected rebound error, but with new sdk instead of rebound it got 409 - this kind of test I have no idea how to fix, so will comment them for author of current library to check:

Expected spy executor.emit to have been called with [ 'rebound', 'Mismatched version for addVariant action on product with masterVariant sku: aMasterVariantReference' ] but actual calls were [ 'error', { body : { statusCode : 409, message : 'A duplicate combination of the variant values (sku, images, prices, attributes) exists.', errors : [ { code : 'DuplicateVariantValues', message : 'A duplicate combination of the variant values (sku, images, prices, attributes) exists.', variantValues : { sku : 'anSku', prices : [  ], images : [  ], attributes : [  ] } } ], originalRequest : { endpoint : '/products/anId', payload : { version : 3, actions : [ { action : 'addVariant', sku : 'anSKU', staged : false, attributes : undefined } ] } } }, code : 409 } ], [ 'end' ]

Another errors kinds funny :) :

Expected 'Internal Server Error' to equal 'Wow! Such error, very problem'.
Expected 'Internal Server Error' to equal 'Ouch'.
Expected 'Internal Server Error' to equal 'Ouch'.

So I can fix all test except with rebound problem, since I do not know what really it should expect.

drobiazko commented 9 years ago

Can you please send the link to the line of code with rebound?

pvoznenko commented 9 years ago

@drobiazko https://github.com/elasticio/sphereio-api/blob/master/spec/actions/updateCustomers.spec.js#L85

test fail message:

  6) Sphereio update customers external id concurrency error should rebound message
   Message:
     Expected spy self.emit to have been called with [ 'rebound' ] but actual calls were [ 'error', { body : { statusCode : 409, originalRequest : { endpoint : '/customers/42', payload : { version : 12, actions : [ { action : 'setExternalId', externalId : '4200' } ] } } }, code : 409 } ], [ 'end' ]
   Stacktrace:
     Error: Expected spy self.emit to have been called with [ 'rebound' ] but actual calls were [ 'error', { body : { statusCode : 409, originalRequest : { endpoint : '/customers/42', payload : { version : 12, actions : [ { action : 'setExternalId', externalId : '4200' } ] } } }, code : 409 } ], [ 'end' ]
    at new jasmine.ExpectationResult (/Users/pavlo/Projects/sphereio-api/node_modules/grunt-jasmine-node/node_modules/jasmine-node/lib/jasmine-node/jasmine-1.3.1.js:114:32)
    at null.toHaveBeenCalledWith (/Users/pavlo/Projects/sphereio-api/node_modules/grunt-jasmine-node/node_modules/jasmine-node/lib/jasmine-node/jasmine-1.3.1.js:1235:29)
    at null.<anonymous> (/Users/pavlo/Projects/sphereio-api/spec/actions/updateCustomers.spec.js:86:31)
    at jasmine.Block.execute (/Users/pavlo/Projects/sphereio-api/node_modules/grunt-jasmine-node/node_modules/jasmine-node/lib/jasmine-node/jasmine-1.3.1.js:1064:17)
    at jasmine.Queue.next_ (/Users/pavlo/Projects/sphereio-api/node_modules/grunt-jasmine-node/node_modules/jasmine-node/lib/jasmine-node/jasmine-1.3.1.js:2096:31)
    at onComplete (/Users/pavlo/Projects/sphereio-api/node_modules/grunt-jasmine-node/node_modules/jasmine-node/lib/jasmine-node/jasmine-1.3.1.js:2092:18)
    at jasmine.WaitsForBlock.execute (/Users/pavlo/Projects/sphereio-api/node_modules/grunt-jasmine-node/node_modules/jasmine-node/lib/jasmine-node/jasmine-1.3.1.js:2576:5)
    at null._onTimeout (/Users/pavlo/Projects/sphereio-api/node_modules/grunt-jasmine-node/node_modules/jasmine-node/lib/jasmine-node/jasmine-1.3.1.js:2590:12)
    at Timer.listOnTimeout [as ontimeout] (timers.js:112:15)
drobiazko commented 9 years ago

Are you talking about this one?

A rebound event is used to tell our runtime to re-deliver the message one more time because of a temporal error. For example, in this particular case 409 Conflict means we are updating a product version which is already updated. By rebounding the message we do a retry with a new version of the product.

drobiazko commented 9 years ago

Ah, I didn't see you answered already. Does my last answer help you?

pvoznenko commented 9 years ago

@drobiazko no I see, it did not handle 409 because of signature of response changed. Thanks for showing me that!

drobiazko commented 9 years ago

If I didn't miss anything, here are all the rebounds:

pvoznenko commented 9 years ago

@drobiazko thanks!

I fixed tests, just checking code before commiting

pvoznenko commented 9 years ago

Ok, one more thing with new SDK library SphereClient default logger does not work, it hangs:

logger = {
            silent: true,
            streams: [
                {level: 'info', stream: process.stdout }
            ]
        };

This works:

logger = {
            path: './sphere-elastic-debug.log',
            levelFile: 'debug'
        };
pvoznenko commented 9 years ago

@drobiazko if to believe this doc logger was removed, is it true?

Talking about this peace of code: https://github.com/elasticio/sphereio-api/blob/master/lib/sphere.js

UPD: yes, as I see from source code, it was removed.

pvoznenko commented 9 years ago

and as I see from the repository in folder 'node_modules' I should push only dependencies not devDependencies?

pvoznenko commented 9 years ago

I believe I fixed issue #62. To make it easy to review my commits, I separate them in two chunks:

So let me know guys what do you think and I am happy to help with Travis or maybe another stuff that I know.

Have a nice evening and best regards, Pavlo Voznenko

drobiazko commented 9 years ago

Thank you @pvoznenko. Our team will review the pull request, test the stuff on our staging system and merge it.

bohdan-chechyn commented 9 years ago

Reviewed, i'll test it and then merge if all will be ok

pvoznenko commented 9 years ago

So related issues #60 and #62 also could be closed?

emmenko commented 9 years ago

@pvoznenko thanks!