Feature/handle network failure in google node

[elvetemedve]

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

Proposed changes

Probably solves issue #189. Errors during token refresh does not always reported back to the caller method. That's why node status is not updated to reflect failed status, but it remains in "querying" state.

To make network communication more fault tolerant, failed HTTP requests needs to be retried multiple times before giving up. Also a delay is added between these retries. In addition to that delay time is increased exponentially after each failed attempt to give a higher chance for the network to recover.

Finally following the "Leave the code in a better state than you found it." principle, I decided to make all spec tests passing (not just google nodes). Running Mocha on the current master branch reports a lot of failing tests. In order to be able to refactor the code or update 3rd-party dependencies more confidently, all tests should be kept in green state, otherwise it's not possible to detect defects brought in my the new changes (in this case updated npm packages does not break functionality).
About half of the test changes are necessary because of Sinon.JS update, see migration guide to v3.x. Rest of the tests are broken because of various reasons.

Changelog

• Update Sinon JS to be able to use newer testing features.
• Fix failing tests.
• Install backoff package to be able to retry failed HTTP requests with increasing delay between them.
• Refactor code to use the exponential backoff strategy for token refresh requests and any Google API requests.

Checklist

• I have read the contribution guidelines
• For non-bugfix PRs, I have discussed this change on the mailing list/slack team.
• I have run grunt to verify the unit tests pass
• I have added suitable unit tests to cover the new/changed functionality

[coveralls]

Changes Unknown when pulling f538db2 on elvetemedve:feature/handle-network-failure-in-google-node into ** on node-red:master**.

[dceejay]

this PR seems good but seems to also address a lot of other tests...
We prefer PRs to do what they say on the tin.
Happy for you to either split into two - or update the description to more fully describe the fixes.
Obviously a larger fix will take longer to review / check over.
Thanks

[elvetemedve]

Splitting the PR would cause one PR dependent on the other. I would rather update the description.

[dceejay]

Sure. Happy to have the contribution either way.

[elvetemedve]

I described the changes I made to tests in the Proposed changes section. I hope it makes sense. :)

Something must be wrong with Travis CI build configuration, because jshint:all started to examine 3rd-party code from node_modules directory (e.g. node_modules/ipaddr.js/ipaddr.min.js). Can this be fixed, please?

[coveralls]

Changes Unknown when pulling 486320f on elvetemedve:feature/handle-network-failure-in-google-node into ** on node-red:master**.

[dceejay]

Sounds good... but when I try it I get

    Potentially unhandled rejection [1] TypeError: sinon.stub(...).callsFake is not a function

multiple times... which file is that declared in ? I can't locate it.

(Also updated the gruntfile to ignore node_modules... thanks for that...)

[elvetemedve]

callsFake() is defined by the sinon pakcage in node_modules/sinon/pkg/sinon-3.3.0.js:931.
Have you updated the packages by npm install? What does npm ls sinon say (sinon@3.3.0 in my case)?

[dceejay]

sweet ! - Happy place - Many many thanks.

[dceejay]

Does this also close #189 ?

[elvetemedve]

Yes, exactly.

[knolleary]

This PR has broken multiple nodes.

• It adds a dependency on the backoff module within the google node, but the google node's package.json has not been updated to include it
• It adds a dependency on the request module within the openweathermap node, but that node's package.json has not been updated to include it - nor does the commit message mention this change
• All of the updates to the top-level package.json are irrelevant as that package.json file is not used by the individual node modules within this repo.

[knolleary]

Just to reiterate, the PR should have been split up to address individual issues.

[elvetemedve]

Regarding point 1) and 2), why Mocha did not catch these mistakes? I would have expected to see failing tests due to missing dependencies (at least on TravisCI, if not on my machine).
3) what's the purpose of the main package.json then?

[knolleary]

The main package.json allows the whole repository to be tested.

But you are right - this has highlighted a need for the tests to do more to verify the packaging of the individual nodes. It currently relies on us spotting such changes when reviewing PRs.