search

teampoltergeist / poltergeist

A PhantomJS driver for Capybara
MIT License
2.5k stars 415 forks source link

attach_file broken for phantomjs version 2.1.1 #866

Closed smellsblue closed 7 years ago

smellsblue commented 7 years ago

I think this issue is cropping up for me with poltergeist 1.13.0 and phantomjs 2.1.1. I noticed this code and suspected it might be the cause. So I patched my installed gem to drop the checks in the compiled JS and use the line return this.click(page_id, id); and it worked.

twalpole commented 7 years ago

attach_file should work just fine with phantomjs 2.1.1 (and it's tested in the test suite) - where did your phantomjs 2.1.1 come from? If you're actually running the phantoms 2.5 beta that will be broken.

smellsblue commented 7 years ago

Installed via Ubuntu 16.04 packages

smellsblue commented 7 years ago

@twalpole can you point me to the spec that should fail if it doesn't work? I can run it on my machine and see if I get a failure

twalpole commented 7 years ago

@smellsblue try using the static linux binary provided by the PhantomJS project. When you run the poltergeist test suite it loads in a whole bunch of tests from Capybara and then runs them using poltergeist as the driver-- the ones that would fail if file upload was broken with 2.1.1 would be in - https://github.com/teamcapybara/capybara/blob/master/lib/capybara/spec/session/attach_file_spec.rb

twalpole commented 7 years ago

And here's the test run using 2.1.1 - https://travis-ci.org/teampoltergeist/poltergeist/jobs/209174674

smellsblue commented 7 years ago

@twalpole I noticed in the travis output you linked me to, I see You're running an old version of PhantomJS, update to >= 2.1.1 for a better experience., even though 2.1.1 is clearly installed earlier... is this possibly indicating Travis is not using 2.1.1 for the specs...?

twalpole commented 7 years ago

@smellsblue Nope -- thats from tests where the returned version is mocked to check for the correct behavior. If you look at the other runs where 1.9.8 is used you'll see there are many more of those warning

smellsblue commented 7 years ago

@twalpole when I ran against Ubuntu 16.04 phantomjs, there are a bunch of failures, including a bunch of attach_file spec failures. I tried installing the same phantomjs used in travis, and the attach_file specs pass (when I set TRAVIS ENV variable to 1 and the PHANTOMJS environment variable to the newly downloaded phantomjs).

Doing some investigation based on that and your comment about using the statically linked phantomjs, I ran across this describing the exact problem. It appears this is fixed in phantomjs by patching the embedded webkit, which isn't done in the Ubuntu package since it dynamically links to the webkit package.

It would be really convenient to be able to use the phantomjs Ubuntu package, but I guess that's not possible unless they update it to use the statically linked binary.

Are you open to some kind of fix in Poltergeist to support the non-statically linked PhantomJS through either a configuration option of some kind, or perhaps somehow detecting the issue dynamically?

Maybe instead of looking at the specific version of PhantomJS to determine if a click is necessary, Poltergeist could test if the value is actually set, and then trigger the click if it is not?

smellsblue commented 7 years ago

(also, thanks for all the help!!!)

twalpole commented 7 years ago

The Ubuntu phantomjs package is broken, and the best solution would be for it to be fixed correctly (for instance, why does it require X to be installed??). We're definitely not interested in any "fix" that requires a config option, and checking the value and then clicking if not set isn't exactly right either since a page could be actively preventing the value from being changed and ending up with two clicks may not be correct. If you really prefer to use the Ubuntu package with all its problems, rather than just downloading the static release binary, you can work around it by calling click on the result of attach_file (which should be the file input element)

page.attach_file('my_file_input', 'some_file_path').click
deivid-rodriguez commented 7 years ago

I just run into this. I didn't know phantomjs in ubuntu repositories was broken. Well, I did know, but apparently I forgot to fix it in my machine...

Anyways, would it be worth adding a note in the README or somewhere explicitly discouraging installation of this package?

twalpole commented 7 years ago

@deivid-rodriguez Sure - PRs are welcome.