Exec testFail instead of exit(3) if channel is not connected

[anderslindho]

Resolves #84

[sveseli]

I am not sure if testFail() is appropriate in the connection callback, as it is not clear which test are we failing. It looks like there is already a test which should fail if monitor is not connected, so it seems to me that removing exit() statement, having some sort of log message in case of failure, and moving event.signal() to the end of the callback function would be sufficient. @anjohnson, what are your thoughts?

[anjohnson]

There looks to be exactly one set of tests which could call this routine, implemented around line 210 of this file. Nothing else uses that ChannelRequesterImpl class, and there's only one channel created so in the current code it is actually clear which test is failing.

However the epicsUnitTest API expects the number of test results generated to always be the same whether they fail or succeed, so replacing the exit(3) with a testFail() breaks that rule since the result of the channel connection is returned to the test() routine and announced in the line testOk1(channelConnected) whether it succeeded or failed.

The author presumably added the exit(3) call in the channelStateChange() method because none of the subsequent tests can be run without a fully connected channel object. This is not the right way to handle that though, the test runner really should be informed that there was a problem detected in the test code.

The best way to handle that situation is to make the event.signal() call in the channelStateChange() method unconditional and remove the else {} block as well, then add an else {testAbort(…)} case to the if (channelConnected) {} statement on line 212. That will fail the testOk1(channelConnected) test and immediately stop the program, telling the user what went wrong.

Another solution would involve moving the following tests that depend on channel into that if (channelConnected) {} block and in the new else {} block call the testSkip() routine with the number of tests which couldn't be run (instead of using testAbort()). That approach is meant for use where there are only a few tests which can't be run but other tests can be performed; in this case that solution would be more effort and can't run any later tests anyway.

If you're seeing this test fail frequently (presumably on a heavily-loaded machine) I would also increase the timeout parameter to the channelRequesterImpl->waitUntilConnected(1.0) method and see if that helps. There are two other …waitUntil…(1.0) calls there which might also need adjusting, but they don't result in exit() being called if they fail.

Thanks!

[anderslindho]

Thanks both. Please see if f903316 looks OK. If it does, I can also drop the initial commit.

I did also add a timeout increase, but I haven't tested if this helps against my spurious failures; I was mainly running into those as part of some other work that I now don't have a strict need to re-run the tests for. I can drop also this commit.

[anderslindho]

Should something more be done here @sveseli @anjohnson ?

[anjohnson]

The CI builds for RTEMS don't like the member assignment on line 141, I think that may need moving into a constructor to be accepted by their older compilers. I haven't looked closely at the code again this time, I will once those builds pass. Sorry for the delay responding…