bug fixes and more testing/coverage #32

Merged
merged 5 commits into from Jan 14, 2017

Conversation

Projects
None yet
3 participants
@tejashah88
Member

tejashah88 commented Jan 14, 2017

  • added testing for request verification, HTTPS support, and POST-based routes
  • fixed potential memory leaks from not closing the HTTPS server instance and not removing the hotswap listeners
  • now using alexa-verifier-middleware for request verification
  • changed loading location of contents of 'sslcert' folder (should be part of 'examples' folder)
  • fixed documentation for generating the SSL certificate
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jan 14, 2017

Coverage Status

Coverage increased (+13.0%) to 77.778% when pulling ab178ce on tejashah88:master into 16f581c on alexa-js:master.

Coverage Status

Coverage increased (+13.0%) to 77.778% when pulling ab178ce on tejashah88:master into 16f581c on alexa-js:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jan 14, 2017

Coverage Status

Coverage increased (+22.1%) to 86.928% when pulling b72dac3 on tejashah88:master into 16f581c on alexa-js:master.

Coverage Status

Coverage increased (+22.1%) to 86.928% when pulling b72dac3 on tejashah88:master into 16f581c on alexa-js:master.

@tejashah88 tejashah88 changed the title from group of bug fixes and more testing/coverage to bug fixes and more testing/coverage Jan 14, 2017

@dblock

dblock approved these changes Jan 14, 2017

I have some minor nitpicks otherwise this looks great. Mind taking a look at the comments? Thanks.

CHANGELOG.md
@@ -2,6 +2,7 @@
### 2.3.2 (Next)
+* [#32](https://github.com/alexa-js/alexa-app-server/pull/32): group of bug fixes and more testing/coverage - [@tejashah88](https://github.com/tejashah88)

This comment has been minimized.

@dblock

dblock Jan 14, 2017

Collaborator

Lets expand this to be specific to be helpful to people reading the CHANGELOG, just list actual fixes on separate lines? Also start with a Capital letter and end with a period ;) <nitpick />

@dblock

dblock Jan 14, 2017

Collaborator

Lets expand this to be specific to be helpful to people reading the CHANGELOG, just list actual fixes on separate lines? Also start with a Capital letter and end with a period ;) <nitpick />

index.js
@@ -7,6 +7,7 @@ var express = require('express');
var alexa = require('alexa-app');
var verifier = require('alexa-verifier');
var bodyParser = require('body-parser');
+var avm = require('alexa-verifier-middleware');

This comment has been minimized.

@dblock

dblock Jan 14, 2017

Collaborator

I would call this alexaVerifierMiddleware to be consistent and remove the var verifier above that i think we no longer need?

@dblock

dblock Jan 14, 2017

Collaborator

I would call this alexaVerifierMiddleware to be consistent and remove the var verifier above that i think we no longer need?

index.js
@@ -23,12 +24,16 @@ var appServer = function(config) {
self.error = function(msg) { console.log(msg); };
// Configure hotswap to watch for changes and swap out module code
- hotswap.on('swap', function(filename) {
+ var swapCallback = function(filename) {

This comment has been minimized.

@dblock

dblock Jan 14, 2017

Collaborator

hotswapCallback? Minor.

@dblock

dblock Jan 14, 2017

Collaborator

hotswapCallback? Minor.

package.json
@@ -20,6 +20,7 @@
"dependencies": {
"alexa-app": "^2.4.0",
"alexa-verifier": "0.0.5",

This comment has been minimized.

@dblock

dblock Jan 14, 2017

Collaborator

We still need an explicit verifier dependency? Maybe not lock to a version and let avm bring it?

@dblock

dblock Jan 14, 2017

Collaborator

We still need an explicit verifier dependency? Maybe not lock to a version and let avm bring it?

This comment has been minimized.

@tejashah88

tejashah88 Jan 14, 2017

Member

good catch for that!

@tejashah88

tejashah88 Jan 14, 2017

Member

good catch for that!

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jan 14, 2017

Coverage Status

Coverage increased (+22.06%) to 86.842% when pulling b005b50 on tejashah88:master into 16f581c on alexa-js:master.

Coverage Status

Coverage increased (+22.06%) to 86.842% when pulling b005b50 on tejashah88:master into 16f581c on alexa-js:master.

@dblock dblock merged commit b622cf1 into alexa-js:master Jan 14, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+22.06%) to 86.842%
Details
@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Jan 14, 2017

Collaborator

Nice work, merged, thank you!

Collaborator

dblock commented Jan 14, 2017

Nice work, merged, thank you!

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Jan 14, 2017

Collaborator

@tejashah88 Want to help maintaining this library? Make a release? I will add you to this repo and npm.

Collaborator

dblock commented Jan 14, 2017

@tejashah88 Want to help maintaining this library? Make a release? I will add you to this repo and npm.

@tejashah88

This comment has been minimized.

Show comment
Hide comment
@tejashah88

tejashah88 Jan 14, 2017

Member

@dblock I would be more than willing to help maintain this library.

Member

tejashah88 commented Jan 14, 2017

@dblock I would be more than willing to help maintain this library.

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Jan 14, 2017

Collaborator

@tejashah88 You're in, you should have both repo and npm access. Maybe you can add something like https://github.com/alexa-js/alexa-app/blob/master/RELEASING.md and then make a release Please continue making pull requests for code, we code review each-others.

Collaborator

dblock commented Jan 14, 2017

@tejashah88 You're in, you should have both repo and npm access. Maybe you can add something like https://github.com/alexa-js/alexa-app/blob/master/RELEASING.md and then make a release Please continue making pull requests for code, we code review each-others.

@tejashah88

This comment has been minimized.

Show comment
Hide comment
@tejashah88

tejashah88 Jan 15, 2017

Member

@dblock No worries, I have quite a few more PR's upcoming! For making the release, should I add the RELEASING.md, or should we make the release first?

Member

tejashah88 commented Jan 15, 2017

@dblock No worries, I have quite a few more PR's upcoming! For making the release, should I add the RELEASING.md, or should we make the release first?

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Jan 15, 2017

Collaborator

I suggest copying/writing RELEASING and doing a release to "test" it. But really that's just my 0.02c, I want to make sure the next person releasing can just follow simple instructions.

Collaborator

dblock commented Jan 15, 2017

I suggest copying/writing RELEASING and doing a release to "test" it. But really that's just my 0.02c, I want to make sure the next person releasing can just follow simple instructions.

@tejashah88

This comment has been minimized.

Show comment
Hide comment
@tejashah88

tejashah88 Jan 16, 2017

Member

Understood, I'll probably be doing some more bug fixing but I do have the RELEASING file ready.

Member

tejashah88 commented Jan 16, 2017

Understood, I'll probably be doing some more bug fixing but I do have the RELEASING file ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment