Testing #99
Testing #99
Conversation
There was a problem hiding this comment.
This is a great start. I left a few specific comments, but they also apply to the rest. With sinon we can make detailed assertions about what we expect our code to do in specific situations. Finding the balance between tests that are too general and too specific takes practice. One rule of thumb is to not make assertions about the specific implementation but to assert the results and side effects you expect as someone calling the function.
package.json
Outdated
| "redis": "^2.7.1" | ||
| "proxyquire": "^1.8.0", | ||
| "redis": "^2.7.1", | ||
| "sinon": "^2.3.2" |
There was a problem hiding this comment.
lets move proxyquire and sinon to devDependencies
There was a problem hiding this comment.
moved them to dev dependencies
test/aws.storage.test.js
Outdated
| conf.bitly_key = null; | ||
| s3Stub.upload.callsArgWith(1, null, {}); | ||
| return storage.set('123', {}, 'Filename.moz', 'url.com') | ||
| .then(reply => assert(reply.url === 'url.com' && reply.uuid !== null)) |
There was a problem hiding this comment.
we should make this into multiple assertions:assert.equal(reply.url, 'url.com') ...
There was a problem hiding this comment.
we can also use sinon to override crypto.randomBytes to return a specific Buffer that we can assert against, something like this:
const buffer = new Buffer(10)
sinon.stub(crypto, 'randomBytes').returns(buffer)
...
assert.equal(reply.uuid, buffer.toString('hex'))
crypto.randomBytes.restore()There was a problem hiding this comment.
Since this function has multiple side effects it's useful to assert that the ones we expect to get called actually were. For example, setting the expire time in redis. We can use sinon to assert that it was called, and with the arguments we expect:
assert(expire.calledOnce)
assert(expire.calledWith('123', 86400000))ref: http://sinonjs.org/releases/v2.3.4/spies/
For this to work you might need to create a beforeEach to call expire.reset() etc.
test/aws.storage.test.js
Outdated
| it('Filesize returns properly if id exists', function() { | ||
| s3Stub.headObject.callsArgWith(1, null, {ContentLength: 1}); | ||
| return storage.length('123') | ||
| .then(reply => assert(reply === 1)) |
There was a problem hiding this comment.
lets prefer assert.equal when possible. It'll print better messages when the assertion fails, including the actual and expected values.
test/aws.storage.test.js
Outdated
|
|
||
| it('Should error when the file does not exist', function() { | ||
| s3Stub.getObject.returns({ | ||
| createReadStream: function() { return null; } |
There was a problem hiding this comment.
this can be a spy that throws to more closely match the actual behavior
|
We should also tweak the .eslintrc.yml file to recognize the mocha specific keywords: diff --git a/.eslintrc.yml b/.eslintrc.yml
index 16f7f63..b98f50c 100644
--- a/.eslintrc.yml
+++ b/.eslintrc.yml
@@ -2,6 +2,7 @@ env:
browser: true
es6: true
jquery: true
+ mocha: true
node: true
extends: |
| @@ -101,7 +103,7 @@ app.post('/delete/:id', (req, res) => { | |||
|
|
|||
| storage | |||
| .delete(id, delete_token) | |||
| .then(err => { | |||
| .then(() => { | |||
| if (!err) { | |||
There was a problem hiding this comment.
I think err is now undefined here.
There was a problem hiding this comment.
Fixed this in a new push!
Finished testing for storage modules