facebookarchive / DEPRECATED-node-wit

Node wrapper for Wit.ai
Other
122 stars 26 forks source link

Add [options] param. #4

Closed irfaan closed 9 years ago

irfaan commented 9 years ago

Options param added to captureTextIntent + captureSpeechIntent calls.

Docs would also need to be updated accordingly.

oliviervaussy commented 9 years ago

Can we add a quick test for this ? With the current test it should be pretty easy to check if the options passed are passed as query params as well,

Otherwise it looks good thanks for the diff

irfaan commented 9 years ago

Sorry - not familiar with testing. I took a look, but from what I understand, you're intercepting the http call and responding with a pre-recorded wit_response, and matching it against wit_response for the test. Is that correct?

Also, how do I check if the options were passed as query params if the query params don't form part of the actual http response for those calls?

oliviervaussy commented 9 years ago

For the test something like that should work :+1:

        it('should send options as given in parameter', function (done) {
            var scope = nock('https://api.wit.ai/', {
                reqheaders: {
                    'Authorization': 'Bearer 1234',
                    'Accept': 'application/vnd.wit.20150306'
                }
            }).get('/message?q=set%20alarm%20tomorrow%20at%205pm&context%5Bverbose%5D=true&context%5Bcontext%5D%5Btest%5D=1')
                .reply(200, wit_response);
            wit.captureTextIntent("1234", "set alarm tomorrow at 5pm", {"verbose": true, "context": {"test" : "1"}},function (err, res) {
                assert.isNull(err, "error should be undefined");
                assert.deepEqual(res, wit_response);
                scope.done();
                done();
            });
        });

The assertion is done using nock, here we assert that the url for the request is the one defined in the .get('') function, so if you add the options it will check that your code is actually adding those options to the request. You can of course add different test: test with undefined options, test with empty object options etc... But the basic test should be something like the one above.

You can add the same kind of test for the speech function as well.

After that your diff will be good to merge, thanks a lot for your help

Catacola commented 9 years ago

Merged and updated by hand. These changes along with tests are now in 2.0.0 and on npm. Closing.

irfaan commented 9 years ago

Whoops! Sorry for the delay! Meant to make those changes + write the test. Thanks @oliviervaussy @Catacola (esp. appreciate the guidance @oliviervaussy).

Catacola commented 9 years ago

It's no problem. Thanks for making this upgrade!