-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Proof of concept fix for #2556 #2557
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added some of my thoughts along the way about my proof-of-concept to seed discussion and weigh caveats.
def submit(method, path, attributes, multipart: false) | ||
browser.submit(method, path, attributes, multipart: multipart) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the easiest way to communicate the multipart
knowledge from the form processing to the browser instance, however this interface change would be breaking if there are any callers which rely on implicit Hash arguments:
def submit(method, path, attributes, multipart: false)
# ...
end
submit("GET", "/", "foo" => "bar") # ! ArgumentError: wrong number of arguments (given 2, expected 3)
submit("GET", "/", {"foo" => "bar"}) # OK
submit("GET", "/", params_hash) # OK
However, the headers sent for a form submission by any browser ultimately do depend on the form value, so I am not sure what would be a superior way to do this.
Perhaps implementing a submit_multipart()
instead?
@@ -88,6 +69,8 @@ def add_input_param(field, params) | |||
|
|||
Capybara::RackTest::Node.new(driver, field).value.to_s | |||
when 'file' | |||
return if value.blank? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have to set up a test case with other browsers to be sure, but I think this kind of short circuit would need to be in place for other <input>
fields with blank values.
I'm not sure if there is in practice a distinction between an empty string value and a nil
value, but the code here already does field['value'].to_s
above.
This would be a breaking change and not one I'm willing to make. If/when rack releases a new version which allows to automatically trigger multipart upload without having to pass the nil upload file then we will support that for those versions, however we will not be dropping current support for existing rack versions. This driver attempts to mimic a real browser but is not 100% the same and isn't guaranteed to be. |
This fixes the issue I reported in #2556.
It would however not post as multipart without another present file unless run against (rack/rack-test#303. A workaround could be to do something like:
when not running under a new enough
rack-test
, if any future Capybara version which might include such a fix needs to maintain compatibility with olderrack-test
s.However, I am not sure which is more problematic: submitting as single part when a
<form enctype="multipart/form-data">
has no filled file inputs or submitting params which were not present in the form in the first place.My gut is that the latter is actually worse because a single-part form with no file fields should only be a problem if a user is asserting the lower-level data submitted -- which is patently not in the spirit of testing with Capybara. The user wouldn't care, nor should any Capybara test, I would think.