search

leonid-shevtsov / headless

Create a virtual X screen from Ruby, record videos and take screenshots.
http://leonid.shevtsov.me/en/headless
MIT License
968 stars 113 forks source link

Fix race condition when starting Xvfb #76

Closed NfNitLoop closed 8 years ago

NfNitLoop commented 8 years ago

This resolves #75

NfNitLoop commented 8 years ago

So, it looks like the ruby 1.x and 2.2 testers failed to install some dependencies.

The ruby2 tests are failing because the specs are trying to fake out the old xvfb_running? by mocking CliUtil methods. But the updated version of xvfb_running? doesn't even get that far. Since the spec is mocking launch_xvfb, @pid never gets set, and xvfb_running? will always return false.

I'm scratching my head a bit at the easiest way to update the tests to handle this.

NfNitLoop commented 8 years ago

Ooh, I see. I accidentally changed the signature of xvfb_running?. It used to return true if there's an Xvfb running for display, even if we didn't start it. Which is the desired behavior if we want to re-use that display. Fixing!

NfNitLoop commented 8 years ago

Yay! Ruby2 tests are passing.

The other 2 environments still seem to be having problems installing their dependencies. I don't think that's due to anything I changed, but please poke me if there's anything I can do to help get this ready for merge!

Thanks for writing Headless!

erkki commented 8 years ago

Anything possible to do here to help with getting this accepted/rejected?

NfNitLoop commented 8 years ago

I'm still around and willing to make any changes necessary to get this merged. (In the meantime, we're just using the fixed version over in https://github.com/NfNitLoop/headless/tree/fix-launch-race.)

erkki commented 8 years ago

Good to know, we're also gonna need to either use a workaround or a custom patch as we're seeing this (or something related) in the wild, so wanted to add an data-point and offer help in any way to get to an resolution.

leonid-shevtsov commented 8 years ago

@NfNitLoop @erkki I'll fix the tests and merge it this weekend.

erkki commented 8 years ago

@leonid-shevtsov much appreciated!

twKrash commented 8 years ago

Any update here?

leonid-shevtsov commented 8 years ago

As it turned out, bundler on Travis was causing issues and that's the only reason the build was failing.

Merged and released in version 2.2.3.