Allow uploading files #110

Merged
merged 2 commits into from Jul 22, 2011

Conversation

Projects
None yet
5 participants
@hubertlepicki
Contributor

hubertlepicki commented Jul 18, 2011

Small patch that allows Capybara's attach_file work as expected. What it does, is that it saves filename passed to attach_file in instance variable and triggers "click" event on input element. Custon chooseFile handler then uses cached filename instead of showing default "choose file" dialog. Works as expected on my projects, i.e. the same behavior as with Selenium driver.

@mike-burns

This comment has been minimized.

Show comment Hide comment
@mike-burns

mike-burns Jul 19, 2011

Owner

Hi @hubertlepicki, thanks for the pull request. HTML5 has support for multiple file uploads; is it possible to support this without breaking compatibility with capybara?

Also this patch needs tests before we can pull it in.

Thanks again,
-Mike

Owner

mike-burns commented Jul 19, 2011

Hi @hubertlepicki, thanks for the pull request. HTML5 has support for multiple file uploads; is it possible to support this without breaking compatibility with capybara?

Also this patch needs tests before we can pull it in.

Thanks again,
-Mike

@hubertlepicki

This comment has been minimized.

Show comment Hide comment
@hubertlepicki

hubertlepicki Jul 19, 2011

Contributor

Mike -- I'll add tests now. Could you point me to some info ref. HTML5 multiple file uploads, I don't now about that?

Contributor

hubertlepicki commented Jul 19, 2011

Mike -- I'll add tests now. Could you point me to some info ref. HTML5 multiple file uploads, I don't now about that?

@hubertlepicki

This comment has been minimized.

Show comment Hide comment
@hubertlepicki

hubertlepicki Jul 19, 2011

Contributor

Actually I changed my mind -- I won't add the tests. This functionality is quite good tested in "session" shared example groups from Capybara, which you have but commented out. I really don't think we need any more tests than that -- or do we? The only failing test is HTML5 multiple file uploads, which I am going to research a bit more and will update pull request when it's passing.

Contributor

hubertlepicki commented Jul 19, 2011

Actually I changed my mind -- I won't add the tests. This functionality is quite good tested in "session" shared example groups from Capybara, which you have but commented out. I really don't think we need any more tests than that -- or do we? The only failing test is HTML5 multiple file uploads, which I am going to research a bit more and will update pull request when it's passing.

@mike-burns

This comment has been minimized.

Show comment Hide comment
@mike-burns

mike-burns Jul 20, 2011

Owner

Details on multiple file uploads in HTML5: http://dev.w3.org/html5/spec/Overview.html#the-multiple-attribute

Perhaps @jferris could comment on the testability of this patch. I'm not as familiar with the infrastructure as he is.

Owner

mike-burns commented Jul 20, 2011

Details on multiple file uploads in HTML5: http://dev.w3.org/html5/spec/Overview.html#the-multiple-attribute

Perhaps @jferris could comment on the testability of this patch. I'm not as familiar with the infrastructure as he is.

@hubertlepicki

This comment has been minimized.

Show comment Hide comment
@hubertlepicki

hubertlepicki Jul 20, 2011

Contributor

I have added support for this HTML5 multiple attribute in file fields. Previous behavior was that QtWebkit was opeining another "Choose file" dialog. This behavior is changed to attach chosen file specified in attach_file now.

@jferris all "attach_file" tests from Capybara's shared examples are passing now, if I uncomment 'it_should_behave_like "session"' line in spec/integration/session_spec.rb -- is this good enough or do we need more tests to be written?

Contributor

hubertlepicki commented Jul 20, 2011

I have added support for this HTML5 multiple attribute in file fields. Previous behavior was that QtWebkit was opeining another "Choose file" dialog. This behavior is changed to attach chosen file specified in attach_file now.

@jferris all "attach_file" tests from Capybara's shared examples are passing now, if I uncomment 'it_should_behave_like "session"' line in spec/integration/session_spec.rb -- is this good enough or do we need more tests to be written?

@jferris

This comment has been minimized.

Show comment Hide comment
@jferris

jferris Jul 20, 2011

Owner

We've been writing our own specs in addition to the ones we get from capybara.

For one, we have potential edge cases in our driver that would reasonably work in every other driver, so there might not be tests for all cases from capybara's examples alone. Also, we all prefer to do things iteratively with TDD, so working from a complete spec isn't a great way to go. Finally, we know there are failures in the shared example groups, so they're not useful for us in terms of detecting regressions.

I've thought about deleting some of our specs once all the built-in capybara ones are passing, but we have a way to go before that can happen.

Short answer: for now, we want our own specs for everything we write, and we can use the shared capybara specs as a compatibility barometer.

Owner

jferris commented Jul 20, 2011

We've been writing our own specs in addition to the ones we get from capybara.

For one, we have potential edge cases in our driver that would reasonably work in every other driver, so there might not be tests for all cases from capybara's examples alone. Also, we all prefer to do things iteratively with TDD, so working from a complete spec isn't a great way to go. Finally, we know there are failures in the shared example groups, so they're not useful for us in terms of detecting regressions.

I've thought about deleting some of our specs once all the built-in capybara ones are passing, but we have a way to go before that can happen.

Short answer: for now, we want our own specs for everything we write, and we can use the shared capybara specs as a compatibility barometer.

@hubertlepicki

This comment has been minimized.

Show comment Hide comment
@hubertlepicki

hubertlepicki Jul 20, 2011

Contributor

I get 15 failures in shared "session" example group, and 2 in "session_with_javascript_support", from which a few seem to be easy to fix. I guess I could work on implementing those. Still not happy to re-implement tests already existing in Capybara's own specs...

Contributor

hubertlepicki commented Jul 20, 2011

I get 15 failures in shared "session" example group, and 2 in "session_with_javascript_support", from which a few seem to be easy to fix. I guess I could work on implementing those. Still not happy to re-implement tests already existing in Capybara's own specs...

@jferris

This comment has been minimized.

Show comment Hide comment
@jferris

jferris Jul 20, 2011

Owner

It's not wonderful to have overlapping coverage, but I'd much prefer that than being unable to do TDD. Even if all the capybara specs were passing, I'd still want our own specs. Not all drivers are the same, and it's nice to be able to see a spec in the project you're working on.

It's awesome that capybara has those shared specs - they're useful in determining compatibility and possible driver issues - but they're not the same as having your own set of tests.

Owner

jferris commented Jul 20, 2011

It's not wonderful to have overlapping coverage, but I'd much prefer that than being unable to do TDD. Even if all the capybara specs were passing, I'd still want our own specs. Not all drivers are the same, and it's nice to be able to see a spec in the project you're working on.

It's awesome that capybara has those shared specs - they're useful in determining compatibility and possible driver issues - but they're not the same as having your own set of tests.

@halogenandtoast

This comment has been minimized.

Show comment Hide comment
@halogenandtoast

halogenandtoast Jul 22, 2011

Contributor

@hubertlepicki I talked with @jferris and convinced him that it's okay to use the built in Capybara tests. However, what I did instead of including 'it_should_behave_like "session" was to instead do it_should_behave_like 'attach_file' and include the extract_results method manually. I'll pull your branch and add the tests in. If everything passes, I'll merge this in.

Contributor

halogenandtoast commented Jul 22, 2011

@hubertlepicki I talked with @jferris and convinced him that it's okay to use the built in Capybara tests. However, what I did instead of including 'it_should_behave_like "session" was to instead do it_should_behave_like 'attach_file' and include the extract_results method manually. I'll pull your branch and add the tests in. If everything passes, I'll merge this in.

@halogenandtoast halogenandtoast merged commit 0c8e347 into thoughtbot:master Jul 22, 2011

@hubertlepicki

This comment has been minimized.

Show comment Hide comment
@hubertlepicki

hubertlepicki Jul 22, 2011

Contributor

Thanks!

Contributor

hubertlepicki commented Jul 22, 2011

Thanks!

@zakwanhaj

This comment has been minimized.

Show comment Hide comment
@zakwanhaj

zakwanhaj Aug 10, 2011

the patch isn't existed in latest version of capybara-webkit gem
http://rubygems.org/gems/capybara-webkit

the patch isn't existed in latest version of capybara-webkit gem
http://rubygems.org/gems/capybara-webkit

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