upload_file(): needs unit tests, and version check #37

Closed
kurmanka opened this Issue Jul 23, 2012 · 11 comments

Comments

Projects
None yet
5 participants
Contributor

kurmanka commented Jul 23, 2012

upload_file() needs unit tests, and needs to check that Selenium is at least 2.8.0, as it doesn't work on older versions. Original ticket context follows.


Java and Ruby client libraries for Selenium Remote Driver have a feature for uploading a file to the host that runs the selenium server. According to this blog post, Selenium server has this feature since version 2.8.0:
http://sauceio.com/index.php/2012/03/selenium-tips-uploading-files-in-remote-webdriver/

The file is uploaded via the /session/:sessionId/file request, which is not documented in JSONWireProtocol, or anywhere else, actually. But the functionality is essential for file upload tests in case you use a 'Selenium-on-demand' service, like the one provided by Sauce Labs.

We could implement an upload_file() method in the Selenium::Remote::Driver class. The method could issue the session/:sessionId/file request (passing the file contents encoded as expected by the server), and would return the remote server's local path to the file uploaded. That path could then be used for a send_keys() call on a correct input element.

My take on how this could be implemented is in the following commit:
kurmanka/Selenium-Remote-Driver@402a0a5

Collaborator

aivaturi commented Jul 24, 2012

Have you tried testing this on Windows, OS X & Linux? Also, can you create a pull request for that patch, that way it is easier for me to pull & test it out. Thanks.

Contributor

kurmanka commented Jul 25, 2012

I've tested on OS X and Linux. There's nothing in the proposed code that is platform-specific, to the best of my knowledge. The only question is how well do the underlying modules perform on Windows.

Yes, I'll figure it out eventually.

Contributor

kurmanka commented Jul 28, 2012

I didn't figure out a way to assign my pull request to a specific issue, sorry. This pull request, I mean: aivaturi#38 and this issue.

Collaborator

aivaturi commented Jul 30, 2012

I just merged your changes in, thank you for your help. There are couple of more things we need to take care of:

  1. Unit tests
  2. On the Sauce Labs page you linked, it states that it is supported from 2.8.0 on-wards. We should make sure that we check the version of Selenium server before executing this command.

brianmed was assigned Mar 28, 2013

gempesaw self-assigned this Nov 25, 2014

Contributor

teodesian commented Dec 31, 2014

Noticing that upload_file against the most recent selenium servers is returning '1' rather than the file path. My preliminary investigation seems to suggest that this is a bug deeper than this module, but I'm still digging. Anyways, unit tests for this might be forthcoming out of this investigation I'm doing.

Owner

gempesaw commented Dec 31, 2014

Ah, hmm... this endpoint is the undocumented one, right? Cool. I haven't used it in a few years, thanks for taking a look!

If you do get around to making unit tests, don't worry about generating the recordings, we can do it right before we're ready to release.

Contributor

teodesian commented Jan 19, 2015

Have resolved issue of not returning file, no_content_success was set incorrectly.
Added unit tests that the method returns the full filename on the remote host, and discovered a quirk of selenium. if the file is in $CWD, it returns the full path. if not, it returns the path to the basename of the file. Going to make upload_file understand this behavior, and 'do the right thing'. Should have a pull request ready later today.

Contributor

teodesian commented Jan 19, 2015

Hmm, I forgot to implement one of the original issues reported by the ticket author, namely to check that the version is correct. What would be the desirable policy on this?

Should we croak, or simply return a false value? I suppose croaking would be consistent with the rest of the module's behavior.

Anyways, I wlll have to write a separate test for behavior across various selenium versions, as each likely requires it's own mockset to be generated, and the harness only supports 1 mockset/test.

If I'm going to do that it might make sense to knock out other version testing as well; that might be it's own 'needs test' issue all by itself. Probably a pretty hefty research task to see which functions exist when.

Owner

gempesaw commented Jan 20, 2015

Honestly, I'm not too concerned about the version check. It made sense back then, but hopefully by now it's a non-issue. I can't imagine someone managing to use 2.8.0, and undocumented functionality, and not being able/knowing how to upgrade their version.

I'd still accept a version check PR if you're so inclined, but it can be separate from this one.

Contributor

teodesian commented Jan 20, 2015

Agree, I think version checks would be nice, especially for some of the new JSONwire stuff. I might get the chance to look at this again in a few weeks, and I'll see about some reasonably comprehensive version tests then.

Owner

gempesaw commented Jan 26, 2015

fixed by #178 and released in 0.23. as previously mentioned, version check PR will be considered separately. Thanks again!

gempesaw closed this Jan 26, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment