Add tests for player

[pkess]

This implements test for the issue #584. It is not finished yet but i will continue working on this

[codecov]

Codecov Report

Merging #1198 (7a20405) into develop (e7a2b04) will increase coverage by 0.17%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1198      +/-   ##
===========================================
+ Coverage    65.65%   65.83%   +0.17%     
===========================================
  Files           86       86              
  Lines         8846     8859      +13     
===========================================
+ Hits          5808     5832      +24     
+ Misses        3038     3027      -11     

[pkess]

I think this is ready for review and merge now

[zariiii9003]

Hello Peter, thank you for your contribution and sorry for the late reply! Tests are always welcome.
I think it makes sense to check the call arguments of the mock. Otherwise this looks good to me, just some minor cosmetic issues.

[pkess]

Thank you for your review. i changed the code now accordingly

[pkess]

Unfortunatelly the compare operator was not implemented for messages. I implemented this now. But do you want this to be added with this pr?

[zariiii9003]

Unfortunatelly the compare operator was not implemented for messages. I implemented this now. But do you want this to be added with this pr?

There is a Message.equals() method for this purpose. I'd say remove your last commit since that is out of scope for this PR.

[pkess]

Unfortunatelly the compare operator was not implemented for messages. I implemented this now. But do you want this to be added with this pr?

There is a Message.equals() method for this purpose. I'd say remove your last commit since that is out of scope for this PR.

I knew about the equals function and i used it in the compare operator. But i think it was necessary to get your proposal with assert_has_calls working. What should i do with this? should i implement an own compare for the executed calls?

[zariiii9003]

I knew about the equals function and i used it in the compare operator. But i think it was necessary to get your proposal with assert_has_calls working. What should i do with this? should i implement an own compare for the executed calls?

I wonder how i did not notice that, sorry. Nevertheless, there was a discussion in #413 and __eq__ was removed.

We could this instead:

        self.assertTrue(self.mock_virtual_bus.send.mock_calls[0].args[0].equals(msg1))
        self.assertTrue(self.mock_virtual_bus.send.mock_calls[1].args[0].equals(msg2))

It's ugly but it does the job.

[pkess]

I knew about the equals function and i used it in the compare operator. But i think it was necessary to get your proposal with assert_has_calls working. What should i do with this? should i implement an own compare for the executed calls?

I wonder how i did not notice that, sorry. Nevertheless, there was a discussion in #413 and __eq__ was removed.

We could this instead:

        self.assertTrue(self.mock_virtual_bus.send.mock_calls[0].args[0].equals(msg1))
        self.assertTrue(self.mock_virtual_bus.send.mock_calls[1].args[0].equals(msg2))

It's ugly but it does the job.

Ok, i implemented it this way now. I used the created message object for the equals function to be sure there will be no error method does not exist when the called argument is not correct.

[pkess]

Unfortunately some tests fail now. i will try to fix this

[pkess]

Unfortunately some tests fail now. i will try to fix this

It looks like the problem is that the args property was introduced with python3.8. So i only compare this in higher versions now. I think this is ok as the behaviour should not change with python version

[felixdivo]

This is a really nice PR! Thanks for tackling this "technical debt" and making sure the code actually works in unit tests! And most importantly, these serve as great regression tests for future changes.

Do you agree that this resolves #584?