Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic sanity check test suite #299

Merged
merged 4 commits into from
Nov 17, 2016

Conversation

joelanman
Copy link
Contributor

based on @andymantell's PR (#292)

this moves the tests from Grunt to Gulp

@joelanman
Copy link
Contributor Author

so this isnt working because in the test, the app doesnt stop - it just keeps listening. I'm working on splitting the server apart from the running of the server, so the test can control the start and stop of the server

gulp.task('mocha', function () {
return gulp.src(['test/**/*.js'], { read: false })
.pipe(mocha({ reporter: 'spec' }))
.on('error', gutil.log)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can use:

  .pipe(mocha({reporter: 'spec'}))
  .once('error', () => {
    process.exit(1)
  })
  .once('end', () => {
    process.exit()
  })

https://github.com/sindresorhus/gulp-mocha#test-suite-not-exiting

@joelanman joelanman force-pushed the andymantell-basic-sanity-check-test-suite branch 4 times, most recently from 574e720 to 187449d Compare November 3, 2016 16:25
Copy link
Contributor

@andymantell andymantell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of small things. App doesn't need to listen in the test script, and Travis is running the suite twice


before(function () {
app = require('../../server')
server = app.listen(3000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the server needs to be listening at this point. supertest can just accept the app variable and handles the listening and un-listening itself. So you can delete var server, server = app.listen(3000) and the entire after() hook and it should still run

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how can supertest make the server close?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my initial research (see the thread above on github) it wasn't closing, and that was causing problems

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joelanman Was that not before you stopped the server.js from doing the listening and put it in run-server? If you try removing that listen, you should see it still works and also stops ok. It seemed to when I tried it a moment ago. To quote supertest:

You may pass an http.Server, or a Function to request() - if the server is not already listening for connections then it is bound to an ephemeral port for you so there is no need to keep track of ports.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -9,7 +9,7 @@
"scripts": {
"start": "node start.js",
"lint": "standard",
"test": "npm run lint"
"test": "gulp test && npm run lint"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Travis is running the test suite twice, because Travis.yml is doing:

script:
- gulp test
- npm test

But npm test already runs gulp test. I would suggest updating Travis.yml to only run npm test

request(app)
.get('/')
.expect('Content-Type', /text\/html/)
.expect(200, done)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you're calling done() in the end method, it needs to be removed from the expect above...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @andymantell, that gets rid of the double callback! message from supertest, I hadn't spotted that.

@gemmaleigh gemmaleigh force-pushed the andymantell-basic-sanity-check-test-suite branch 2 times, most recently from b7903ee to 2c4eafe Compare November 16, 2016 13:13
andymantell and others added 4 commits November 16, 2016 15:12
App doesn't need to listen in the test script as supertest accepts the
app variable and handles the listening and un-listening itself.

This also removes the needs for the after block to stop the server.
@gemmaleigh gemmaleigh force-pushed the andymantell-basic-sanity-check-test-suite branch 2 times, most recently from 4e4523b to 49965cc Compare November 17, 2016 10:07
@robinwhittleton
Copy link
Contributor

Looks good to me. Thanks @andymantell!

@robinwhittleton robinwhittleton merged commit e4378ed into master Nov 17, 2016
@robinwhittleton robinwhittleton deleted the andymantell-basic-sanity-check-test-suite branch November 17, 2016 11:58
@gemmaleigh gemmaleigh mentioned this pull request Nov 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants