Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for windows x64 machines #69

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pf-bajorek
Copy link

  1. Phatnomjs works for x64 machines so it doesn't have to be validate by architecture.
  2. To find phanotm path we use whereinstead of which

Fix for windows x64 machines
@colszowka
Copy link
Owner

@pf-bajorek Thanks - the build is failing though because of the changed where/which line for the windows tests. Could you fix that test please?

@pf-bajorek
Copy link
Author

@colszowka Hi!
I checked travis and failed examples is:

rspec ./spec/platform_spec.rb:176 # Phantomjs::Platform on Windows with system install returns the correct phantom js executable path for the platform

The error encourage because it is executed on linux machine:

Called from **/home/travis/**build/colszowka/phantomjs-gem/spec/platform_spec.rb

I don't know whether trevis can run tests on windows?

What do you do to fix this test? Skip it on unix machines?

@colszowka
Copy link
Owner

The test suite is built to simulate the corresponding platforms by stubbing out the respective root calls, see the setup block of the failing test in https://github.com/colszowka/phantomjs-gem/blob/master/spec/platform_spec.rb#L177 :)

@pf-bajorek
Copy link
Author

@colszowka done :)

@pf-bajorek
Copy link
Author

So, will it be merged?

@pf-bajorek
Copy link
Author

ping

@pf-bajorek
Copy link
Author

@colszowka Are you planing to merge this issue to master and push new gem version?

@karlhe
Copy link

karlhe commented Jul 19, 2016

I would suggest 2 additional changes.

To account for more windows setups (mine identifies as mswin32):

        def useable?
          host_os =~ /mingw|mswin|cygwin/
        end

To account for multiple phantomjs.exe on path:

        def system_phantomjs_path
          `where phantomjs`.split("\n")[0]
        rescue
        end
``

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants