Next gen #4
Next gen #4
Conversation
.travis.yml
Outdated
| - export TWILIO_API_KEY=Your-Twilio-API-KEY | ||
| - export TWILIO_API_SECRET=Your-Twilio-API-SECRET | ||
| - export TWILIO_IPM_SERVICE_SID=Your-Twilio-IPM-SERVICE-SID | ||
| - NODE_TLS_REJECT_UNAUTHORIZED=0 |
There was a problem hiding this comment.
This line is not really necessary and makes the code quite unsafe. You can read about it here nodejs/node#5258 (comment)
.travis.yml
Outdated
| - TWILIO_IPM_SERVICE_SID=Your-Twilio-IPM-SERVICE-SID | ||
|
|
||
| cache: | ||
| apt: true |
There was a problem hiding this comment.
It doesn't seem we are installing anything with apt, what is the purpose of this line?
| @@ -56,6 +59,7 @@ a service like [ngrok](https://ngrok.com/). | |||
| $ ngrok http 3000 | |||
| ``` | |||
|
|
|||
|
|
|||
| ## Run the tests | |||
|
|
|||
| 1. Run backend tests | |||
There was a problem hiding this comment.
As a suggestion, you could change mocha test for npm test and also in package.json prevent the environment variable from being exported to console session and remove the && operator, as it's not necessary.
app.js
Outdated
| @@ -16,10 +16,10 @@ app.set('views', path.join(__dirname, 'views')); | |||
| app.set('view engine', 'jade'); | |||
|
|
|||
| // uncomment after placing your favicon in /public | |||
| //app.use(favicon(path.join(__dirname, 'public', 'favicon.ico'))); | |||
| // app.use(favicon(path.join(__dirname, 'public', 'favicon.ico'))); | |||
There was a problem hiding this comment.
Lines 18 and 19 can be deleted as most likely they won't be used and if they are needed, they can be recovered from history
routes/token.js
Outdated
| @@ -2,7 +2,8 @@ var express = require('express'); | |||
| var router = express.Router(); | |||
| var TokenService = require('../services/tokenService'); | |||
|
|
|||
| // POST /token/generate | |||
|
|
|||
| // POST /token | |||
test/token.js
Outdated
| describe('token', function () { | ||
| describe('POST /token', function () { | ||
| it('generates a token', function (done) { | ||
|
|
There was a problem hiding this comment.
I think we could remove this extra line
package.json
Outdated
| @@ -4,7 +4,7 @@ | |||
| "private": "private", | |||
| "scripts": { | |||
| "start": "node ./bin/www", | |||
| "test": "export NODE_ENV=test && mocha test" | |||
| "test": "NODE_ENV=test npm test" | |||
There was a problem hiding this comment.
In this line you define what the command npm test do. If you write here npm test you are creating a loop and that's why your build is broken. Please replace back with mocha
There was a problem hiding this comment.
Additinally, you should rely on the mocha version installed for this project, concretely in ./node_modules/mocha/bin/mocha test.
There is another subtle problem. You are relying on dotenv-safe, which means that you don't have any step where you instruct the user to export the required env variables.
To fix this problem you have to explicitly export the env vars in the test script.
"test": "NODE_ENV=test TWILIO_ACCOUNT_SID=ACXXXXXXX ...."For referencing mocha please use ./node_modules/.bin/mocha.
package.json
Outdated
| @@ -3,8 +3,8 @@ | |||
| "version": "1.0.0", | |||
| "private": "private", | |||
| "scripts": { | |||
| "start": "node ./bin/www", | |||
| "test": "NODE_ENV=test npm test" | |||
| "start": "npm start", | |||
There was a problem hiding this comment.
What's the reason for this change? It was fine before, but now you are referencing npm start from npm start, creating an endless loop
|
thanks @juancarlospaco ! Looks good to me |
package.json
Outdated
| "moment": "^2.11.1", | ||
| "morgan": "~1.7.0", | ||
| "serve-favicon": "~2.3.2", | ||
| "twilio": "3.0.0-rc.16" |
There was a problem hiding this comment.
You could add ~ in front on the version so that it's not locked to this RC.
⊂(ʘ‿ʘ)つ