search

strongloop / strong-supervisor

Application supervisor that automatically adds cluster control and performance monitoring with StrongOps
Other
66 stars 20 forks source link

test: update ws test to new control-channel api #149

Closed sam-github closed 9 years ago

sam-github commented 9 years ago

depends on strongloop/strong-control-channel#32 connected to strongloop-internal/scrum-nodeops#775

Ryan, I copied this test-ws-errors over to control-channel essentially verbatim, maybe it should be removed from strong-supervisor?

rmg commented 9 years ago

@sam-github the purpose of that test was to ensure that strong-supervisor did the right thing when the channel told it that there was an error. If that is something that supervisor is no longer responsible for, then removing it seems reasonable.

As for moving it to strong-control-channel, unless it was removed, strong-control-channel already has its own version of these tests.

rmg commented 9 years ago

@sam-github I've looked at the PR and I'm not seeing what the actual change is, other than a bunch of restructuring required because on('connection') is no longer used. Was removing that part of the changes that were made upstream?

sam-github commented 9 years ago

Upstream doesn't emit events when it reconnects its internal websocket anymore, so I had to rework the test.

I removed your test-ws-errors from control-channel, it was too hard to get working because it relied so heavily on observation of internal state, and was much longer and more complex than this test here. I moved its error injection code into test-ws-reconnects, though, which appeared to test the same condition, but turned out to be a bit different.

Once I changed this test to work again, it was trivially rewriteable to not need to fork, I don't see anything in it now that isn't tested upstream.

In particular, thanks for jogging my memory, supervisor shouldn't handle errors anymore, they occur only on fatal errors, it should exit when they occur. I posted fixup.

sam-github commented 9 years ago

So, basically, test-ws-errors exists again upstream, a copy of this test minus the fork(), and supervisor is no longer responsible for reconnecting on ws error, the control channel does that reconnection internally, so its not clear there is anything left here that is being tested.

rmg commented 9 years ago

Oh, well if supervisor isn't expected to do anything on WS error, then that entire test can be removed IMHO. It was added explicitly for testing that supervisor handled the error.

sam-github commented 9 years ago

ok, cool, thanks.

can you review https://github.com/strongloop/strong-control-channel/pull/32?

rmg commented 9 years ago

strongloop/strong-control-channel#32 has been LGTM'd

sam-github commented 9 years ago

@slnode test please

sam-github commented 9 years ago

@slnode test please

sam-github commented 9 years ago

@slnode test please

rmg commented 9 years ago

LGTM.