search

cgoldberg / xvfbwrapper

Manage headless displays with Xvfb (X virtual framebuffer)
Other
295 stars 52 forks source link

Add display argument #38

Closed jreusch closed 5 years ago

jreusch commented 5 years ago

Adding possibility to define the display to start on. Inspired by pull request #37, will fix issue #23.

Compared to PR #37 i changed the position of the argument to not break the interface for whoever would use the tempdir argument. Also the PR introduced a bug that when a display was specified and it was already locked, it would end up in an infinite loop.

Also added a test and an explanation to the readme. Besides that fixed(?) a bug, in the locking method when an actual Xvfb is running. The tests which currently handle this do not start an actual instance and thus behave differently.

Additional question: is a ValueError the one we would desire when a display is specified but it is already locked? I did not come of with a more suitable one, but maybe ppl have other wishes/opinions about it?

jreusch commented 5 years ago

should have fixed the python2 issues...

cgoldberg commented 5 years ago

should have fixed the python2 issues...

@jreusch just checking if these are fixed in your branch? .... python2.7 tests pass for me locally and also on travis-ci. so it looks good to merge... let me know and I'll merge it.

jreusch commented 5 years ago

should have fixed the python2 issues...

@jreusch just checking if these are fixed in your branch? .... python2.7 tests pass for me locally and also on travis-ci. so it looks good to merge... let me know and I'll merge it.

that comment was ment for commit 459879b, so yes, i see them solved. so yes, good to merge IMHO, if you agree with the ValueError exception. :)

cgoldberg commented 5 years ago

Alternatives to ValueError might be RunTimeError or defining a custom exception class. However, I think this is fine. Merging as-is.

jreusch commented 5 years ago

great! thanks for the quick merge and the whole wrapper in the first place! :)