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

update dependencies #4

Merged
merged 4 commits into from Mar 14, 2018
Merged

update dependencies #4

merged 4 commits into from Mar 14, 2018

Conversation

cklanac
Copy link
Contributor

@cklanac cklanac commented Mar 13, 2018

  • updated dependencies
  • Removed --watch. It clears the node cache so modules are reloaded but resources like connections are not always dropped so connections can build up and we run out, especially on ElephantSQL.
  • Add --exit to force http and database connections to close after tests.
  • Use --file option to specify a file to run first.

- Removed --watch. It clears the node cache so modules are reloaded but resources like connections are not always dropped so connections can build up and we run out, especially on ElephantSQL.
- Add --exit to force http and database connections to close after tests.
- Use --file option to specify a file to run first.
@oampo
Copy link
Contributor

oampo commented Mar 14, 2018

👍 On bumping the dependencies

I'm not totally sure I understand why we need the changes in to the Mocha commands.

It exits fine my end without adding --exit - isn't this the point of the dbDisconnect in after? Is the issue that we don't have a closeServer any more, which should also be in after?

If (and that's quite a big if) the code is cleaning up after itself properly then we shouldn't see connections being held open right? So it seems like if that isn't working we should fix that rather than dropping --watch?

--file looks good for setup.test.js, but I'd like to keep some variant of the */*.test.js rule so students can keep their tests inline with their src if they want, or have them in the separate test directory. That brings us inline with create-react-app, so we can teach one set of rules.

@cklanac
Copy link
Contributor Author

cklanac commented Mar 14, 2018

RE: --exit I agree with you, it would be better to fix the tests so resources are closed properly and mocha exits. But the course doesn't currently explain that Mocha won't exit if you don't close open processes. So --exit is a temporary solution to prevent tests from hanging in Travis CI.

RE: --watch I misspoke/miswrote, the --watch doesn't release event listeners which causes issues.

"(node) warning: possible EventEmitter memory leak detected. 11 listeners added. Use emitter.setMaxListeners() to increase limit."

I encountered this when trying to add graceful exits to the project. This issue has been around for a long time but not fixed. mochajs/mocha/issues/117 And there is or at least was a proposal to deprecate it. mochajs/mocha/issues/1780

If you want to keep watch functionality, I think nodemon --exec mocha... is more reliable

RE: --file and keeping */*.test.js. The node course doesn't put the tests inline, yet... and I forgot that create-react-app does.

@oampo
Copy link
Contributor

oampo commented Mar 14, 2018

That all makes a lot of sense. I'll add the inline tests option back in, then merge. TBH this all makes me more keen to switch to Jest for everything...

@oampo oampo merged commit 250b913 into master Mar 14, 2018
@cklanac cklanac deleted the update-dependencies branch March 14, 2018 12:48
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.

None yet

2 participants