Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

automatic JSON parsing of result from #evaluate_script not always a good thing #283

Closed
wants to merge 1 commit into from

2 participants

@sdhull

I get that it's generally pretty convenient to have that string be JSON parsed, but once in awhile I just want the raw string back (especially when trying to see that js confirmations have the correct text).

Anyway if that call to JSON.parse fails, then you're out of luck. This commit simply rescues with the string that was fed to JSON.parse (the raw result of the evaluate_script call).

Just for some context, this is what I wanted to do in my project:

match = page.evaluate_script(%Q|$('#offer-#{counter_offer.id} input[value="Accept"]').attr('onclick').toString()")|)
match.should include(expected_message)
@sdhull

FWIW I do have a workaround, I just don't like it. Plus it's kind of a matter of principle. Ideally I think that json-parsed result should be a different method or maybe the same method but like evaluate_script("4+4", :parsed => true) or something. Or maybe leave it up to users to implement their own JSON-parsing wrappers around #evaluate_script. Ultimately though, it should return the value you asked for.

@jferris
Admin

Thanks for the patch.

The driver is using JSON as an implementation detail, and doesn't expect your evaluate_script call to return a JSON string.

A better solution here would be to make sure we generate valid JSON. In the event that evaluate script returns a string, it's generating JSON like ["some string"], which is perfectly valid, and does return the raw string back. In your example, it looks like the webkit server is not properly escaping your string, so it's failing to parse.

Your test is a good start, but the implementation is just covering a wider problem, so I'm not going to pull this in as-is.

@jferris jferris closed this
@sdhull

Sorry about the late response, various other things at work had higher priority than fixing this.

"The driver is using JSON as an implementation detail" -- not sure what you mean here. Are you saying that the fact that it's using JSON.parse is unnecessary?

"doesn't expect your evaluate_script call to return a JSON string." -- I would argue that it absolutely does expect your evaluate_script call to return a valid JSON string (as is evidenced by the failing spec without the rescue).

All I can do is recommend that capybara-webkit allow users to wrap the result of evaluate_script in JSON.parse manually instead of doing it internally. After all, that would represent some sort of parity with the selenium driver.

Until then, having evaluate_script break whenever invalid JSON is returned is really not great. Rescuing the JSON.parse call is indeed not so good, but arguably better than letting JSON.parse raise an exception.

Just my 2c.

@jferris
Admin

More accurately, the driver isn't supposed to expect the evaluate_script call to return valid JSON. The idea is that it converts whatever object your return to JSON in the server, writes it to the socket, and then the driver parses it and returns it as native Ruby objects.

The server code that serializes results into JSON is choking on whatever object your example is returning. The correct fix would be to figure out why, and fix the server to properly serialize that object.

@sdhull

I've taken a peak at the server code you're talking about (I guess I was a bit remiss in not taking a look sooner).

So I take it that your point here is that if my call to evaluate_script were to return markup (like the result of inner html) or the text of a javascript function, then the server code should properly escape it so that JSON.parse doesn't break, and instead returns a string. Right?

Is there some CPP JSON library we could use to construct the json, instead of doing it manually as it's currently being done? I imagine an existing library would have worked out all the possible escaping issues (which is what I think is causing problems in my case).

@jferris
Admin

There are existing JSON libraries in C++. The best one I was able to find for Qt was this: http://qjson.sourceforge.net/

However, it depends on cmake and people already have a decent amount of trouble compile capybara-webkit with the existing dependencies. If anybody manages to get a better JSON parser in there, I'd be interested. I don't think I'll have time to mess with it myself in a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 28, 2012
  1. @sdhull
This page is out of date. Refresh to see the latest.
Showing with 8 additions and 1 deletion.
  1. +1 −1  lib/capybara/driver/webkit/browser.rb
  2. +7 −0 spec/driver_spec.rb
View
2  lib/capybara/driver/webkit/browser.rb
@@ -97,7 +97,7 @@ def command(name, *args)
def evaluate_script(script)
json = command('Evaluate', script)
- JSON.parse("[#{json}]").first
+ JSON.parse("[#{json}]").first rescue json
end
def execute_script(script)
View
7 spec/driver_spec.rb
@@ -96,6 +96,13 @@
end
end
+ it "returns a string if JSON parsing fails" do
+ subject.within_frame("f") do
+ result = subject.evaluate_script(%<document.getElementsByTagName('script')[0].textContent>)
+ result.should == "\"\n document.write(\\\"<p id='farewell'>goodbye</p>\\\");\n \""
+ end
+ end
+
it "executes Javascript" do
subject.within_frame("f") do
subject.execute_script(%<document.getElementById('farewell').innerHTML = 'yo'>)
Something went wrong with that request. Please try again.