Better handle startup failures - do not leave dangling processes

[matthijskooijman]

Description

This PR fixes #196, by making sure that if any startup failures occur after the omxplayer process is started, that the omxplayer process is killed before raising an exception. This prevents a situation where an omxplayer might be running in the background, without any reference to its pid to kill or control it.

• Status: *READY**

Todos

• [X] Tests
• [ ] Documentation (Not needed I think, except maybe a changelog?)

Steps to Test or Reproduce

Without this commit, if you let DBusConnection.__init__ raise DBusException("Foo") and then start a player, the constructor will throw an exception but leave omxplayer running in the background.

[codecov[bot]]

Codecov Report

Merging #201 into master will increase coverage by 1.77%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
+ Coverage   72.05%   73.82%   +1.77%     
==========================================
  Files           6        6              
  Lines         458      470      +12     
  Branches       25       26       +1     
==========================================
+ Hits          330      347      +17     
+ Misses        117      113       -4     
+ Partials       11       10       -1     

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 625a396...932e1db. Read the comment docs.

[matthijskooijman]

w00ps, misclicked the close button.

Anyway, based on the code coverage report, I added three more testcases (one amended into the second comment, two unrelated ones added in a separate commit). Code coverage should now improve rather then be reduced by this PR :-)

[willprice]

Thanks for this @matthijskooijman, it looks really good. I don't have much time this week to look at this. I'd like to test it out myself before merging. I'll try and do that next week.

Thanks again for your contribution!

[matthijskooijman]

I don't have much time this week to look at this. I'd like to test it out myself before merging. I'll try and do that next week.

I know very well how that works, so don't worry (but thanks for replying anyway!) :-)

[willprice]

Thanks again @matthijskooijman. This is great and thanks for putting the effort into writing the tests :+1:. I've pulled down your branch and run the tests and things seem good (although there are a few spurious errors in the integration tests, I don't think they're anything to do with your changes, I'll have a look into them separately).

Sorry it's taken me so long to look at this, but I hope to merge this in this week :)