tejashah88 / node-alexa-smapi

A node.js client library for using the Alexa Skill Management API.
MIT License
13 stars 3 forks source link

support for v1 with initial testing #4

Closed marcelobern closed 6 years ago

marcelobern commented 6 years ago

Added:

marcelobern commented 6 years ago

@tejashah88 in this latest review I added support for Travis CI with the following results for the Nodejs versions below:

Please let me know if you want me to drop earlier versions of Nodejs so we can get the Travis CI build passing (Nodejs 9 & 10 for now).

tejashah88 commented 6 years ago

@marcelobern I'd prefer to have support down till node 4 (since that's when native support for promises began), but I can live with up until node 6, since it won't be EOL until April 2019.

Concerning the TooManyRequestsException, it could be due to rate limiting if the response code is error 429, in which you'll have to put some sleeping functions to delay subsequent requests.

Other than that and the merge conflicts, we are close to releasing v1.

marcelobern commented 6 years ago

@tejashah88 I am open to ideas around sleeping approaches which would work on Mocha.

I tried's Nodejs setTimeout() and it went no where. The only way I could make it work was using

function sleep(waitTime) {
  // TODO figure out a cleaner way to add a wait time
  Atomics.wait(new Int32Array(new SharedArrayBuffer(1024)), 0, 0, waitTime);
}

BTW, I now have passing test cases for:

So it would be great if we could release support for SMAPI v1 as is - with the Travis CI failing for Node 6 & 7 (my last build passed Node 8 through 10) and come out with 1.0.1 right away adding these new test cases and giving us time to figure out the sleep function approach for Node 6 - 7.

Also, do you already have this project setup on Travis CI and Coveralls?

tejashah88 commented 6 years ago

I've setup Travis CI and coveralls.io integration, so it should work once this PR is merged.

I'm perfectly fine with releasing v1 ASAP, since it supports the latest versions of node for now. It's more of a nice to have for node 6 and 7 support at this point until someone asks about it.

Have you tried using setTimeout inside a beforeEach clause or promisifying setTimeout and calling it between API calls? I'm curious as to why setTimeout wasn't working for you.

marcelobern commented 6 years ago

You can see below the options I tried below.

Keep in mind the use case here is not only to sleep between calls but also to retry while we wait for builds to complete. Right now we retry up to 10 times if status === 429 or when needing to give SMAPI time to work (e.g. after skill create/update/certification, interaction model update).

Summary of my observations when exploring the options in the code below:

/* eslint-env mocha */
'use strict';

const MAX_RETRIES = 3;
const RETRY_TIMEOUT = 1000;

var chai = require('chai');
var chaiAsPromised = require('chai-as-promised');
chai.use(chaiAsPromised);
var expect = chai.expect;

describe('Testing sleep() approaches', function() {
  this.retries(MAX_RETRIES);

  describe('-> using Atomics.wait', function() {
    var subject;

    beforeEach(function() {
      subject = Promise.resolve(false);
    });

    it('checking time elapsed', function() {
      subject = subject.then(function(response) {
        const start = Date.now();
        console.log('---> #1-Will go to sleep <---');
        Atomics.wait(new Int32Array(new SharedArrayBuffer(1024)), 0, 0, RETRY_TIMEOUT);
        console.log(`---> #1-Rise and shine, I slept for ${Date.now() - start}ms <---`);
        return response;
      });
      return expect(subject).to.eventually.become(true);
    });
  });

  describe('-> using setTimeout', function() {
    it('setTimeout with done, no promises', function(done){
      const start = Date.now();
      console.log('---> #2-Will go to sleep <---');
      setTimeout(function() {
        console.log(`---> #2-Rise and shine, I slept for ${Date.now() - start}ms <---`);
        done();
      }, RETRY_TIMEOUT);
      expect(false).to.equal(true);
    });
  });

  describe('-> using setTimeout', function() {
    var subject;

    beforeEach(function() {
      subject = Promise.resolve(false);
    });

    it('setTimeout with promises', function() {
      subject = subject.then(function(response) {
        const start = Date.now();
        console.log('---> #3-Will go to sleep <---');
        setTimeout(function() {
          console.log(`---> #3-Rise and shine, I slept for ${Date.now() - start}ms <---`);
        }, RETRY_TIMEOUT);
        return response;
      });
      return expect(subject).to.eventually.become(true);
    });
  });

  describe('-> using setTimeout', function() {
    var subject;

    beforeEach(function() {
      subject = Promise.resolve(false);
    });

    it('setTimeout with promises & done', function(done) {
      subject = subject.then(function(response) {
        const start = Date.now();
        console.log('---> #4-Will go to sleep <---');
        setTimeout(function() {
          console.log(`---> #4-Rise and shine, I slept for ${Date.now() - start}ms <---`);
          done();
        }, RETRY_TIMEOUT);
        return response;
      });
      return expect(subject).to.eventually.become(true);
    });
  });
});
tejashah88 commented 6 years ago

So I was playing with the option 3 code and I got it to pause as expected.

describe('-> using setTimeout', function() {
  var subject;

  var sleep = (ms) => new Promise(resolve => setTimeout(resolve, ms))

  beforeEach(function() {
    subject = Promise.resolve(false);
  });

  it('setTimeout with promises', function() {
    subject = subject.then(function(response) {
      const start = Date.now();
      console.log('---> #3-Will go to sleep <---');
      return sleep(RETRY_TIMEOUT).then(() => {
        console.log(`---> #3-Rise and shine, I slept for ${Date.now() - start}ms <---`);
        return response;
      });
    });
    return expect(subject).to.eventually.become(true);
  });
});

The basic idea is that we have sleep, which is a promisified version of setTimeout. By returning a sleeping promise which chains into whatever actions are to be done, it allows the promise chain to be fully resolved once it returns the response. You may want to test it and clean it up if it works for you. Cheers!

marcelobern commented 6 years ago

@tejashah88 thanks a bunch for the assist, that did it!

tejashah88 commented 6 years ago

@marcelobern Great! I'll release what we have right now as v1.0.0