Fix connection-related leaks and inconsistencies

[baffo32]

Fixes #92

Notable changes:

• Added a utils test file that tracks setTimeout() and setInterval() uses that are never cleared.
• Changed .connected to .isConnected() so that it may be part of the public API
• Paired uses of .sntpStart with matching .sntpStop
• Raise a few more errors in places where exceptional things happen
• Handled all forms of disconnection and cleaned up state

[andrewjaykeller]

jaw dropped on the awesomeness that is this pr. just reviewed all the code and it all looks great Karl! Just get those pesky tests passing :) and I'm very happy with this!

[andrewjaykeller]

It's looks like one of your should is not passing in the constructor test.

[baffo32]

it looks like it's because the SNTP is timing out or failing occasionally for that test on travis.

The second failure is reporting that SNTP's setInterval is still running after the test has ended because .sntpStop was not called

should probably be handled more robustly somehow. SNTP is rather a kludge anyway right now because the two driver objects reference the same sntp object and will alter each other's state

[andrewjaykeller]

Agreed, I think there is a lot of clean up that needs to happen with sntp

[codecov-io]

Current coverage is 94.10% (diff: 97.52%)

Merging #111 into 1.4.0 will increase coverage by 0.32%

@@              1.4.0       #111   diff @@
==========================================
  Files             4          4          
  Lines          2232       2255    +23   
  Methods         129        132     +3   
  Messages          0          0          
  Branches        471        479     +8   
==========================================
+ Hits           2093       2122    +29   
+ Misses          139        133     -6   
  Partials          0          0          

Powered by Codecov. Last update c59cd18...35aa651

[baffo32]

because of https://github.com/hueniverse/sntp/issues/14 (sntp.start does not report errors), I added a call to sntpGetOffset, which does report errors, in sntpStart to determine if the options are valid, which adds another 2 seconds to construction time. had to increase timeouts

[baffo32]

yowch. I can't reproduce these timeouts locally. Guess I'll need to learn how to set up travis so I can debug this without adding lots of commits to an open pull request.

[andrewjaykeller]

I just looked at the error and saw it's a different one then before. It does look like a timeout, failure to establish the sntp sync, issue that could be causing it. It's been working though so I'm more interested in what was changed within the sntp function to make (expose?) the tests fail.

[baffo32]

In order to add coverage, I added a test for sntpStart rejection in the constructor.

But it's never hit because of a bug in sntp (https://github.com/hueniverse/sntp/issues/14).

So I added a call to sntpGetOffset into sntpStart and used it to detect failure. This is why it takes so long.

[andrewjaykeller]

Gut feeling that test/.openBCIBoard-test.js.swp should be added to the gitignore seeing as it's a binary.

[andrewjaykeller]

it's looking good so far!!!

[baffo32]

same test failure once on my end https://travis-ci.org/baffo32/OpenBCI_NodeJS/builds/169840347

[andrewjaykeller]

Jeeze this is super inconsistent

[andrewjaykeller]

Karl, is there anything i can do to help you?

[baffo32]

looks like it's intertwined with some set of runaway impedance promises. it only fails when they line up with the parallel sntp. I'll look into it more later

[andrewjaykeller]

hmm i tried to pull out most of the impedances into a separate testing file, could you please link to the suspected line?

[baffo32]

https://travis-ci.org/baffo32/OpenBCI_NodeJS/jobs/169840481#L1297 these impedance-related outputs are some delayed output from the impedance testing file, which was run much earlier. some impedance process was ongoing when some impedance test decided it didn't need anything more and ended early.

I was thinking maybe of upgrading to a more complex promise library, to track down things like this, but I don't think it is necessary

[baffo32]

For the ones that fail, the impedance-related output is generated during the test that fails: https://travis-ci.org/baffo32/OpenBCI_NodeJS/jobs/169840488#L1736 so they are probably related

[baffo32]

@aj-ptw I've gotten the tests passing. I pulled in bluebird (for tests only) to track unresolved promises, which revealed a lot of them, all of which I plugged. Any thoughts?

[andrewjaykeller]

Changed .connected to .isConnected() so that it may be part of the public API

Should we do the same for streaming?

[andrewjaykeller]

which revealed a lot of them, all of which I plugged. Any thoughts?

everything I'm seeing seems to only drastically increase the quality of promise test coverage. Looks good, I don't think it's decrememnting quality of coverage. Not too sure how to verify this though. I'm happy to merge this with 1.4.0 and begin testing it

[baffo32]

Failing test is #115 again EDIT: guess it's no longer failing =)

[andrewjaykeller]

Thanks @baffo32 !