Merge index tests #91
Conversation
…it was affected. I put it back in.
| // This makes a server request to the route location '/' | ||
| chai.request(server) | ||
| .get('/') | ||
| .end(function (err, res, body) { |
There was a problem hiding this comment.
There is a parameter body here, but it's never used. Do we need it?
There was a problem hiding this comment.
Nope, you're right; if it does nothing then we don't need it.
| let chai = require('chai'); | ||
| let expect = chai.expect; | ||
| let should = chai.should; |
There was a problem hiding this comment.
lint complained about removing should
There was a problem hiding this comment.
nvm this was resolved when I changed the next note to expect
|
Apart from rogue should, LGTM. |
| .post('/') | ||
| .field('username', fixture.username) | ||
| .field('password', fixture.password) | ||
| .field('teamName', fixture.teamName); |
There was a problem hiding this comment.
Did the linter accept all of the extraneous whitespace? Tabs should be 2 spaces
There was a problem hiding this comment.
Maybe try linting the branch locally?
There was a problem hiding this comment.
GitHub is still showing the rogue should. So no, that's unresolved
There was a problem hiding this comment.
@chances I wasn't sure if expect.fail() is correct. It worked, but it wasn't in the docs I saw it on a site.
There was a problem hiding this comment.
@ibilal expect.fail() is fine, see: chaijs/chai#487 (Does expect.to.fail90 work?)
|
Before you merge this branch, I noticed that I get this printed to the console and I'm not sure if it's supposed to be doing this: |
|
@ibilal Another feature branch was merged with leftover console log's in the index get request test. |
|
@chances doesn't look like there are any console logs in that test. Might be a warning from node about an actual double callback? |
Merged all index GET/POST tests into a single file.