Fix test failures

[dlrdave]

Some tests were failing because:

One of the mtsCommandVoidReturn Execute overloads was calling itself, and resulted in infinite recursion. Similary for one of the mtsCommandWriteReturn Execute methods.

Fixed by changing the implementation to call another signature of the Execute method.

Are these two changes the correct fix for this particular badness in the code...?

TestCleanup was failing because:

CPPUNIT_ASSERT_EQUAL is used to verify managerLocal.ComponentMap.size() == 0, but the Cleanup method does not remove any components, so the size is still 1...

Is this a bug in the test?

Or is Cleanup supposed to call some "remove" method on items left in that map...?

This pull request assumes it is a legitimate result and fixes the test to pass with size() == 1.

TestAddComponent was failing because of this code:

https://github.com/jhu-cisst/cisst/blob/4f705501e6c2db09766e116e4c7f964ebfe1d38e/cisstMultiTask/code/mtsManagerLocal.cpp#L1353-L1358

ManagerComponent.Client is NULL during the test, because AddManagerComponent is never called, because CreateManagerComponents is never called, because GetInstance is the only caller of it, and GetInstance is not used for the test.

Similarly for many other mtsManagerLocalTest tests.

These were all fixed by adding calls to CreateManagerComponents directly to each test. (Seems that adding/removing any components, and connecting any interfaces requires the creation of the manager components...)

TestGetComponentNames was still failing after fixing that because:

Manager component names being in the list now, too. Reworked test method to account for that.

TestConnectDisconnect still fails because:

mtsManagerComponentClient::Connect calls GetInstance to do some work, and so it uses a different instance than we set up as a local var in the test...

So: tried to change the test to use GetInstance.

But it still failed, because mtsManagerComponentClient::Connect sets a local var result to true, and never sets it to false. Is the test wrong, or is the method wrong because it does not return false over several different code paths?

This pull request does not yet fully address this test failure.

TestConnectLocally still fails because:

Similar to TestConnectDisconnect...

The comment /*, bool & result*/ appears all over the place with respect to connect calls. How is this test ever supposed to detect connect failures if failures are never set in the code...?

This pull request does not yet fully address this test failure either.

[pkazanzides]

These changes are definitely an improvement.

The mtsManagerLocal::Cleanup method probably should remove all components from ComponentMap, but it would be risky to make this change. One of the biggest weaknesses of cisstMultiTask is in the cleanup, where the order of destroying/cleaning up is not always correct and can lead to crashes on program exit. Thus, I agree that it is better to change the test.

[pkazanzides]

I also agree with the manual call to CreateManagerComponents, since GetInstance is not called. I was not involved in creating these tests, so I find it odd that many of the tests bypass the singleton and directly create an instance of the mtsLocalManager class. The manager component services was a later addition to cisstMultiTask, so that is why the tests did not originally call it (though it is surprising that nobody tried to fix this earlier).

[minyang]

@pkazanzides It has been quite a while since I wrote the very early version of unit test suites for cisstMultiTask. Not sure how substantial changes cisstMultiTask and its unit test suites have gone through since then. My vague recall around bypassing the singleton in unit tests is that it was needed to get around some issues with singleton at that time, something related to clean up of the singleton instance. Because all unit tests are run in the same process, there was no way to test a "clean" instance of mtsManagerLocal (created via singleton instantiation) in all unit tests except the very first one. If there is a way to cleanly release and recreate the singleton instance, it would be definitely a better way to write unit tests. Just a few words from historical memory.

[pkazanzides]

Yes, I eventually figured this out when revising the tests. Nevertheless, I was able to get most tests to work using GetInstance -- see the feature-mts-new branch. I added a RemoveAllUserComponents method to mtsManagerLocal to help with this, though I'm sure not everything is completely cleaned up. In particular, I think the connection map is not properly cleaned.

After doing this, I thought of another way to get things to work, which is to create the local instance of the class and then provide an mtsManagerLocal::SetInstance method, so that later calls to mtsManagerLocal::GetInstance would return the proper object.