Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

[bug] Gulp test wasn't running server tests [closes #873] #891

Merged
merged 1 commit into from
Sep 26, 2015

Conversation

codydaig
Copy link
Member

@codydaig codydaig commented Sep 6, 2015

This PR fixes #873. The models were never getting loaded in the gulp tests so it was freezing on mongoose errors. This loads the models before running the server tests.

@codydaig
Copy link
Member Author

codydaig commented Sep 6, 2015

@mikepc

@mleanos
Copy link
Member

mleanos commented Sep 7, 2015

I can confirm this fixes the issue.

@codydaig It looks like the behavior between the Grunt & Gulp test tasks is slightly different. Using Gulp, it appears that the karma task is started before the Mocha task completes. This ends up skewing the reporting output from the tests. Whereas, using Grunt, the Karma task doesn't start until the Mocha task is complete.

@mleanos
Copy link
Member

mleanos commented Sep 7, 2015

@codydaig It looks like the Gulp test task is running both the Karma & Mocha tests in parallel.. The task defined here https://github.com/meanjs/mean/blob/master/gulpfile.js#L222, can be updated to this

// Run the project tests
gulp.task('test', function (done) {
  runSequence('env:test', 'mocha', 'karma', done);
});

This will run the Mocha task first, and then run the Karma task. Learned something new about run-sequence :) anything that you want to run in parallel, you just group them within an array param of runSequence() But in this case, I think we want Mocha to run first; to be consistent between Grunt & Gulp.

@codydaig
Copy link
Member Author

codydaig commented Sep 7, 2015

@mleanos I'm working on refactoring the gulp file to be more inline with the grunt file. This was just a bug fix in the meantime. I'm happy to make the change if thats desired in this PR as well.

@mleanos
Copy link
Member

mleanos commented Sep 7, 2015

@codydaig I think making that change in this PR is appropriate. Just so that this particular Gulp task outputs the results in a readable manner.

@codydaig
Copy link
Member Author

codydaig commented Sep 7, 2015

@mleanos Change has been made.

@mleanos
Copy link
Member

mleanos commented Sep 7, 2015

Perfect. LGTM. Thanks @codydaig

@codydaig
Copy link
Member Author

@rhutchison Any comments on this PR? I think you we're one of the guys pushing for gulp.

@ilanbiala
Copy link
Member

@codydaig regarding what were you looking for feedback from @rhutchison?

@codydaig
Copy link
Member Author

@ilanbiala I'm pretty sure it was @rhutchison that was pushing for the gulp change, and had made some improvements in the gulp file in the past. But I've tested this and it makes it so you can run the tests with gulp, so since ryan doesn't have any feedback, then I think it's good to merge.

@ilanbiala ilanbiala added this to the 0.4.2 milestone Sep 26, 2015
@ilanbiala ilanbiala self-assigned this Sep 26, 2015
ilanbiala added a commit that referenced this pull request Sep 26, 2015
Enable running tests through Gulp
closes #873
@ilanbiala ilanbiala merged commit 48b1a9c into meanjs:master Sep 26, 2015
@codydaig codydaig deleted the bug/gulphang branch September 27, 2015 21:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gulp mocha/gulp karma hang on execution
3 participants