Add BEST_EFFORT in the controller switch tests.

[Xi-HHHM]

Hi,

I am not sure if this PR meets the requirement of issue #521. A short review or feedback will be helpful.

[bmagyar]

So... these tests are quite bulky. What I meant with the original comment of trying to parametrize them is to make it a little more compact. Can you think of a way to refactor this and the existing tests such that you can only pass in which mode you are testing, BEST_EFFORT or STRICT and some expectations on what should be the outcome? We are looking for something to reduce the code.

[destogl]

@Xi-HHHM this is very good first step! Thanks!

Along the comments from @bmagyar we should now try to make those test shorter. This concrete cases, where only one controller is “switched” has basically the same functionality for both cases, and therefore you could simply add a new function in the test file and call with different argument, i.e., STRICT and BEST_EFFORT.

I imagine the following tasks to do in this PR:

1. Rename also another test to explicitly state that STRICT is used.
2. Add additional tests were multiple controllers are “switched” so that we can really test different behaviors between STRICT and BEST_EFFORT. In short, the meaning of those by “switching” multiple controllers is:

• STRICT - is any of controllers can not be “switched”, none is “switched”
• BEST_EFFORT - any controller that can be “switched” is “switched”

@bmagyar: Do you agree? Is this correct?

[bmagyar]

Yep, let me just take this out of draft mode so we can see the pipeline results.
I think @Xi-HHHM has done the basic job of what was described on the ticket ;)

There is one thing I'd note: these interfaces are meant to have different behaviour for some cases, why are both happy with testing exactly the same assertions?
2 things:

• the behaviours are not different -> bug
• the tests are not triggering anything that'd lead to different behaviour -> follow-up issue, specific ticket for this?

@Xi-HHHM what is your take on this?

[Xi-HHHM]

Only one controller is used and switch on and off in the test case. I think it was the reason why both test returned the same result.

What we can do is that we add a "fake_controller", which is not defined and loaded, into the list of start_controllers or stop_controllers and expect different outcomes.

What do you think? @bmagyar @destogl

[Xi-HHHM]

In this version, I added multiple controllers for testing and defined the expected behavior of BEST_EFFORT and STRICT. I hope the test was implemented properly.

I notice one thing though. As I tried to start a controller which has already been started, the test will crash(not fail). A hypothesis is that there is a resource conflict between the controllers. And then, I noticed that the test will crash as well, if we want to start a controller, and the a controller of the same type has already started. That is why I left the commented code cm_->configure_controller(TEST_CONTROLLER2_NAME);
there for further discussion.

[bmagyar]

I think there's nothing further to discuss! Can you please proceed with investigating whether the crash is the fault of the test or you've found a bug in controller_manager?

[bmagyar]

@Xi-HHHM any updates on this?

[Xi-HHHM]

I think there's nothing further to discuss! Can you please proceed with investigating whether the crash is the fault of the test or you've found a bug in controller_manager?

@bmagyar Sorry for replying late. I will go head and investigate what caused the crash.

[bmagyar]

The CI is now passing... am I correct that somewhere within that merge master to... you commented out the crashing part?

[Xi-HHHM]

The CI is now passing... am I correct that somewhere within that merge master to... you commented out the crashing part?

Yes. The crashing part was commented out in that commit.

Updates in the new commit:

• The bug was finally found and it was the problem of the test itself.
• The current implementation follow this pipeline:
  1. Configure and add two controllers: test_controller and test_controller2
  2. Start the fake_controller and test_controller2, where the fake_controller is just a string and should not be found anywhere
  3. Call update, check the returned status of the controller_manager and internal counter of the test_controller2
  4. Start test_controller, call update, check the returned status and the internal counter
• The third step expected difference values of returned status and the internal counter.
  • STRICT: test_controller2 should not be started, controller manager should return ERROR and the internal counter should be zero.
  • BEST_EFFORT: test_controller2 should be started although the fake_controller can not be found. The controller manager should return OK and the internal counter should be greater than one.

[destogl]

@bmagyar @Xi-HHHM what do you think that we merge this now?

@Xi-HHHM and I talked yesterday about further steps and that would be:

• split current test into "lifecycle test" and "activating multiple controllers when one is unknown"
• add test for "activating multiple controllers when one has a resource collision"

[destogl]

@bmagyar @Xi-HHHM what do you think that we merge this now?

@Xi-HHHM and I talked yesterday about further steps and that would be:

• split current test into "lifecycle test" and "activating multiple controllers when one is unknown"
• add test for "activating multiple controllers when one has a resource collision"

[destogl]

This is now fixing existing test. I would merge this and do the follow-up work in the new PR.

[bmagyar]

agreed