Elements, nil, and functions throwing NPE's #85

Closed
seancorfield opened this Issue Nov 6, 2012 · 25 comments

Projects

None yet

4 participants

@seancorfield

It would be much nicer from a testing perspective if this just returned nil so you just get a test failure instead of an exception (and a giant stack trace).

The workaround is to use (when-let [item (first (elements "..."))] (text item)) but that's kinda ugly all over your test code :(

And it would also be convenient if (text nil) returned nil so you could do things like (text (second (elements "..."))) without needing conditionals.

Owner
semperos commented Nov 6, 2012

I'm open for discussion on this one, but here are my thoughts.

Part of the reason to use Selenium-WebDriver at all is to have a level of assurance that elements you interact with exist and are, if necessary, visible. The fact that you get an NPE with clj-webdriver in this case is an artifact of my own workflow, wherein I catch Selenium's built-in NoSuchElementException class to allow things like my own exists function to be a function (instead of macro). My opinion is that it is exceptional to try to do something with an element that isn't actually there, so it should throw an exception, though I agree that the NPE is sub-optimal.

As a final point, returning nil from functions like text for non-existent elements would be ambiguous. Would it mean that the element isn't there, or that it's text was empty? Ostensibly empty text would be returned as the empty string "", but I think it would still cause confusion.

(text "...") => "" when the element is empty, (text "...") => nil when the element is missing. Having a truthy result when the element is present and a falsey result otherwise just seems more idiomatic to me.

I'd rather have (is (= "Some text" (text "..."))) fail with "Expected Some text, actual nil" than an NPE (and a giant stack trace).

Whilst I would agree that attempting to interact with an element that should be there but isn't is exceptional when you're programming a web browser, I don't agree it's exceptional when writing tests and throwing an exception makes it much less pleasant to use webdriver with clojure.test or expectations.

I guess I could always write my own text-if-present function but that is also going to lead to ugly tests, compared to plain ol' text (and, yeah, I could exclude your text and provide my own but then my ns declaration is going to look nasty and it'll be too easy to forget and get the default text function instead).

Owner
semperos commented Nov 7, 2012

I think I understand where you're coming from, but I think there are still some sticky points. I'm not dead-set against the idea, especially since Clojure definitely leans towards returning nil for non-existent things (a good example being functions that deal with hash maps) and eschews returning lots of exceptions. I just want to talk out the edge cases that are specific to Selenium-WebDriver before I make any sweeping changes.

If we were to do this for text, then we'd need to be consistent and do it across the board for functions that deal with elements. Some cases to consider:

What about functions with side-effects like click, which already return nil lest they try to return an element that existed on a previous page (i.e. a clicked link), which will throw an exception at a low level in Selenium-WebDriver, or in our discussion would return nil? We could improve click by allowing an optional boolean argument that says "return the element I just clicked, because I know I'm not leaving the page," in which case we could return the element if successful or nil if the element weren't there. But then all code that used click would have to check click's return value to ensure the element was actually clicked and the side-effect accomplished, whereas now click is used just for its side-effects.

What about functions like attribute, that could return nil because the element wasn't there, or nil because you asked for an attribute that wasn't there?

I'm not trying to be combative; I'm genuinely interested in solving this issue. No one likes unhelpful exceptions and stacktraces.

Totally understand about the general case being problematic. I'll look at our tests in more detail and see if there's another "natural" option that could be generalized more easily...

Owner
semperos commented Nov 7, 2012

I think one straightforward, faithful-to-Clojure win could be: return nil for functions without side-effects, throw an exception for ones with side-effects.

Asking for attributes or text of an element should have the same semantics as querying a Clojure map, for which nil values already carry a lot of ambiguity (compare (:foo {:foo "bar"}) with (:foo {:baz "bam"}) and (:foo nil)). That said, every method call in Selenium-WebDriver is a client-to-server call over an HTTP-based RESTful API between your client Selenium-WebDriver code and a running WebDriver server that controls the browser, so there really aren't any pure functions. If something really squirrelly were going on, catching exceptions and returning nil might silently swallow a meaningful, low-level exception. But I think we could try the catch-all and nil return in practice and see what folks find (that's what pre-1.0's are for).

Once we enter the realm of side-effects, I think folks are used to dealing with a bit more brittleness, in which case we could keep exceptions, just like you'd get a FileNotFound exception if you tried to interact with a non-existent file.

I'll start exploring this for the next beta (beta3) iteration. Thanks for the feedback; let me know if you think of other aspects for handling this.

That sounds like a good approach. I'll experiment locally and see if I have any other ideas!

bonega commented Dec 1, 2012

Disclaimer - very little knowledge of the projects architecture.

We can get nil-fns without swallowing exceptions.

Have a look at this example code: http://pastebin.com/sy3g7Ngz
Missing elements will return nil, but we still get exceptions.

All operations should be composed from 'element' as the most basic building block.
So probably all fns acting on 'element' should return 'nil' for missing element.
I would make 'click' return 'true' for completion.

'elements' should return '[]' for seq-testing
(when (seq list-of-elements)
(println "I am not an empty list"))

Questions? I tend to be a bit unclear with my explaining sometimes.
I would probably be willing to contribute some code.

Owner
semperos commented Dec 5, 2012

Contributed code always welcome :)

The example in that paste looks nice. I'll need to spend some time with it implemented and work through the cases described above.

Owner
semperos commented Feb 4, 2013

Upcoming Changes in Clojure Core

So when this issue was first raised, I didn't actually remember that core Clojure functions returned nil in places where, for example, you try (get a b) where a isn't an associative data structure; I thought it would throw an exception in that example. And so I started writing a long response in this issue about how returning nil for missing elements would create confusion, subtle bugs and inconsistencies, until I went through core Clojure functions and realized I was wrong.

But today, I feel a little vindicated in my original assumptions. I'd like to draw attention to Stuart Sierra's recent post and JIRA issues:

It looks as though Clojure is going to be cutting back on the level of nil punning that goes on, such that using things like contains? or get on non-associative data structures will throw an exception when passed a non-associative argument instead of just returning nil like it does in versions <= 1.4.0.

Impact on clj-webdriver

This obviously raises issues for the approaches discussed here.

If we think of the web page as an associative data structure, functions like the find-element function should return nil when looking for an element that doesn't exist. The same as doing (get {:foo "bar"} :baz) returns (and will continue to return) nil.

However, things change when we talk about functions that work on individual elements (i.e., the Element record). Elements, like the page, are associative data structures. When I fix the above, and functions like find-element return nil for a non-existing element, functions that expect to work on elements (text, click, attribute, etc.) will no longer be getting an associative. Under Clojure behavior for versions <= 1.4.0, this would return nil, but under behavior for the contains? function in upcoming 1.5.0, and in other functions that work on maps in future Clojure releases (like get, whose behavior many of clj-webdriver's functions mimic), an exception will be thrown instead.

I agree with these behavioral changes. Severe type mismatches should be considered exceptional behavior, and should not be the source of hard-to-find null-related bugs elsewhere in the app by propagating this behavior.

This seems to bring us full circle. The most basic workflow in clj-wedriver is:

  1. Find an element using selector system of choice
  2. Do something to that element

Under clj-webdriver's current implementation, where {:webelement nil} is returned for non-existent elements, this results in a NullPointerException being thrown at the Selenium-WebDriver level. This is bad, for one, because the source of the nil is not immediately obvious.

If we change it to return nil for non-existent elements, then we should be throwing an exception like Stuart Sierra wrote into the new contains? functions, along the lines of "<function name> not supported on type: " + coll.getClass().getName()" in order to be compatible with upcoming (and more correct) behavior.

But that seems almost as bad as the NullPointerException, because it's not related to clj-webdriver's domain. The problem is you're trying to do things to an element that doesn't exist; the fact the return value of find-element does not implement Associative is, in my opinion, an implementation detail and the wrong level of abstraction for errors in clj-webdriver's API.

Proposal

I think the following would satisfy folks' pain points and keep us in the spirit of better Clojure core behaviors.

  • Make functions that work on the page return nil if an element is not found (i.e., (get {:foo "bar"} :baz) ;=> nil)
  • Make functions that work on elements throw a custom clj-webdriver exception if they get nil or any non-associative data structure as an argument (i.e., new (contains? '(1 2 3) 1) ;=> IllegalArgumentException)

It would be much better to get something like a NoSuchElementException for (text (element "not-here")) than either the current NullPointerException or even IllegalArgumentException that new Clojure behavior would suggest. The only reason I was willing to give in earlier was because I did not want to create a departure from core Clojure behaviors in this library, but now that they are aligning with my expectations, I do not intend to make this kind of code simply return nil.

Implementation

I think this could be done as simply as extending the appropriate clj-webdriver protocols to nil and throwing an aptly-named exception, perhaps NoSuchElementException. For any other non-associative data structures, Clojure's protocol error handling would be triggered because they wouldn't have protocol implementations. Current data structures that participate in element-related protocols are the Element record and clojure.lang.IPersistentMap.

This approach, of course, will only work once I stop returning the gross {:webelement nil} nonsense for non existent elements.

Throwing a custom exception will be preferable to the current NPE and definitely reasonable to handle in test code (although slightly less convenient than nil :) My real objection was to NPE.

Owner
semperos commented Feb 4, 2013

Thanks for the feedback. I agree, the NPE is loathsome.

Owner

I'm working on the implementation for this right now. As for the OP's original issue of having when-let forms throughout test code, I'm confused as to how a test shouldn't fail if an element you expected to be there wasn't found? And having an exception related to an element not being found, versus having a test simply fail because you thought an element should have text "foo" but it was "", hides the real problem.

It's a matter of cleanliness and being able to read test failures. The NPE stack trace makes the test failures much harder to read. The when-let approach allowed me to avoid the stack trace and instead get a nice, controlled test failure instead. Having a specific exception thrown by the library will allow me to write wrapper functions to unwind to nil - and get a clean test failure - without having to deal with "random" exceptions.

I'd still prefer to get nil back, to be honest, because I like the "maybe" pattern and in Clojure 1.5.0 I'll probably rely on some-> quite a lot. But I can define a set of wrappers that at least map your specific exception to that pattern.

Hope that clarifies?

Owner

This and #90 fixed with release 0.6.0-beta3

@semperos semperos closed this Feb 28, 2013
bonega commented Jun 9, 2013

I am sorry, but I feel the need for further discussion on the matter.

I am using clj-webdriver for scraping.
That entails a lot of elements that perhaps are missing by design or just haven't loaded by some reason.
I absolutely mustn't halt the program because of a missing element.

Solution(?) wrap every operation in try/catch or when-let.
Firstly this is very tiring, and secondly it is badly out of sync with how Clojure handles associatives.

I do a lot of something close to this: http://pastebin.com/h6npFc6F

What does Clojure say?

By design "get" will return nil if we pass nil.

(get nil :somekey)
nil

The proposed change to Clojure is if we actually pass something to "get".
An exception is only thrown if the structure passed isn't nil and isn't associative.

https://twitter.com/lantiga/status/298443715184320513

So (let [e (element "missing")](text e)

Is supposed to return nil if we follow Clojure's example.

What types of subtle bugs are we talking if we return nil?

I agree in principle that getting nil back is far more preferable to an exception but at least with a known, custom exception I could use wrappers and get nil back again, and continue on my Clojure-idiomatic way.

I was never able to upgrade to beta3 because of problems with the underlying Selenium library and haven't had time to investigate beyond that.

FWIW, I just tried 0.6.0 which is using the 2.31.0 Selenium library and so far my test suite seems stable so it looks like whatever the earlier problem was with beta3's version of that library has since been fixed.

Update on 0.6.0 and 2.31.0 Selenium library - definitely not stable for me. I see exactly the same problems that prevented me upgrading to 0.6.0-beta3. So I'm back on beta2 and that's where we'll stay until the Selenium library is fixed.

bonega commented Jun 11, 2013

Sean: Please report that as an issue, seems important on its own.
I have some stability problems with Firefox, but not with the Chrome-driver.
You could try that aswell.

Owner

@seancorfield Sorry to hear that the Selenium-WebDriver upgrades are not working. I don't recall from our discussions -- have you tried to simply exclude the selenium-server dependency from clj-webdriver in your project.clj/pom.xml file and then depended directly on the version you know works for your codebase? I don't believe I've used any features unique to the latest version of selenium-server, so that might be worth a try if you want to get the improvements to clj-webdriver itself. Let me know if there are incompatibilities in the clj-webdriver codebase with that earlier version of selenium-server and I'll see if I can't work around them.

@bonega switching to another browser might be a good solution but I haven't gotten around to trying that seriously across all our dev/CI environments.

@semperos that's probably worth trying. I did at one point create a build of 0.6.0-beta3 with the older Selenium WebDriver library and that worked fine (which is how I isolated it to the WebDriver).

Owner

@seancorfield You shouldn't have to rebuild clj-webdriver, you can just use the :exclusions key in your dependency entry for clj-webdriver and exclude the selenium-server jar, and then explicitly include it (at the version you need) as a direct dependency in your code base.

I know. I'm just saying that last time to try this out - per your suggestion at the time - I built clj-webdriver locally with the old version of Selenium and verified it worked. This time I'm going the :exclusions route - I already exclude core.cache because last time I checked you were using a version that was incompatible with our app. I'll let you know how it goes (the first test run passed but then it was a couple of runs in before I started seeing the failure to populate form fields that plagued me with later Selenium JAR versions before).

I still have the problem in 0.6.0, of when i call (click "foo") and foo isn't on the page, I just get a generic NoSuchElementException on nil.

The thrown exception needs to say something like "foo" not found. But it doesn't. Every time there's an error I have to dig through the stacktrace and the source to figure out what element was missing.

click does a find, and then the low level click action. If the find returns nil, click needs to throw an exception immediately, not just blindly pass nil to the lower level click action (which it knows can't possibly work).

And implementing the protocol on nil doesn't help either, because the original query is lost by then.

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