Hotfix/execution bugs

[tiwanari]

I checked #3 and fortunately the issue was (almost) solved 🎉

However, unfortunately, there are many bugs in some classes:

• 1st commit
  • ExecutionThread: The Program Count hadn't been updated in a while loop.
  • BlockProgramLogic: Some return clauses returned opposite values.
• 2nd commit
  • BluetoothCommunicator: This is not a bug but a notice was lacking and this made it difficult to know whether the second exception occurred or not (related to the 4th commit below).
• 3rd commit
  • ExecutionActivityBase: A request code setting method hadn't been called and thus the class didn't finish after executions.
• 4th commit
  • Remove is_connected flag that was uglily scatted in many places. It was introduced to avoid useless reconnection to a device but some code changes, especially around MachineController, had conflicted the mechanism and caused connection errors. So, I forced ExecutionActivityBase to connect and disconnect (this was lacking in the old code) before / after execution respectively.

[tiwanari]

Sorry, I forgot adding null checks and to commit it (yeah, actually they are related to #3).

[mandaiy]

I did not check thoroughly your changes yet, but could you give here how you have tested your changes? I guess you did on a real robot and your tablet but in case you tested something other than on them.

And from my quick look, it seems easier to write test code for BlockProgrammingLogic. For NxtController if you remove the logic related to preferences, the class might be a test-friendly class.

[tiwanari]

Thanks for your comments :) I'll add more detail. Can you give me a moment?

[tiwanari]

Let me leave a note. Actually, as you said, most of the tests are running some programs. If you expected some test programs, to tell the truth, there were no concrete tests for them.

Firstly, I executed a blank program. At this point, my NXT hasn't sensors nor motors and the program successfully finished. This meant NxtControllerBuilder has finished his life as for motors because ExecutionThread makes commands to NXT motors after execution even if the program was blank. This lacks tests for sensors and I noticed NxtController has no null checks (this is the latest commit). With the change, #3 were surely solved.

However, ExecutionActivity didn't finish after execution, i.e., I couldn't go back to ProgrammingActivity. So, I dug into some classes such as ExecutionCondition because there is a possibility that the program was caught in a forever loop and, of course, I checked ExecutionActivityBase. I found most of the bugs manually in this stage.

The 4th commit is totally different from others. When I tried to run a program twice or more after fixing other bugs, I always caught a connection error. Tracing an execution, I found NxtMachine (mMachine) in NxtExecutionActivity hadn't been initialized and it caused an Exception. The lack of initialization was caused by the replacement of our old library with new one drivecommand and I decided to remove all uglily is_connected flags.

[tiwanari]

So, please give me a little time to add tests as you advised :)

[tiwanari]

Hi, I added some tests and fixed bugs.

BTW, how about using PowerMock? I have trouble testing final classes because they cannot be mocked/spied with Mockito.

Note: I don't like Whitebox.foo() in some test codes which is a part of Mockito ...

[tiwanari]

Thanks for comments, @myusak :)

So, I'll try to introduce PowerMock and make some more tests 💪

[tiwanari]

Hmm... I have trouble incorporating PowerMock because of a bug of DexMaker, poor compatibleness between PowerMock and DexMaker and so on :cyclone:

In short, using PowerMock along with DexMaker is impossible at present because of incompatibility (How to get Powermock to work with Dexmaker - Stack Overflow). Therefore, we cannot use PowerMock in androidTest.

[tiwanari]

Hi @myusak,

I fixed codes based on your advice, however, I couldn't use PowerMock at present because of the problem I mentioned above. This does not permit me to remove Whitebox and throw an Exception when null is passed to BluetoothCommunicator in order to do other tests because BluetoothDevice cannot be mocked and I should pass null to it in other tests.

At this point, I use @NonNull annotation as a workaround (compromise) but we may as well introduce a wrapper such as BluetoothDeviceProvider that can be mocked easily and thus enables testing BluetoothCommunicator.

What do you think? Should we do that in this PR? If so, I will also implement such class for NxtController as you pointed out.

[mandaiy]

I believe that actual code comes first before test code. I think the BluetoothComunicator should reject null object in its constructor and for now you may as well skip the test code. When the problem of Powermock is solved, we'd go for that.

I cannot agree with introducing such wrapper interface, since users of the communicator don't want to be bothered by that. Feel free to criticise me if my understanding is wrong.

[tiwanari]

Thanks for your comments :) I agreed and commented out all tests related to PowerMock. So, please review codes again 🙇

[tiwanari]

Hi @myusak,

Thank you for helpful comments :satisfied: I'll follow your advice :)

[tiwanari]

I fixed codes.

[tiwanari]

Hi @myusak,

I modified codes based on your helpful comments :star: I keep some topics discussed here as cards in Release project such as Guava and Finalizer.