Add Session/Element.requireElement #157
Conversation
tristanlabelle
requested review from
z2oh,
jeffdav,
stevenbrix,
vinocher-bc and
a team
| webDriver: WinAppDriver.start(), // Requires WinAppDriver to be installed on the machine | ||
| desiredCapabilities: WinAppDriver.Capabilities.startApp(name: "notepad.exe")) | ||
| session.findElement(locator: .name("close"))?.click() | ||
| try session.requireElement(locator: .name("close")).click() |
There was a problem hiding this comment.
why not just change findElement to return a non optional?
There was a problem hiding this comment.
- A goal of mine is for the API to support the common scenario of negative queries without having to handle thrown errors. i.e. "don't use errors for control flow". It also has the benefits of being less noisy if you're debugging with first-chance exceptions
- The name
findElementis neutral and I'm trying to call out thatif try? findElement() == nilis a bad idea because it will wait the full timeout in the expected scenario. There might be other ways to achieve this, curious for your thoughts
There was a problem hiding this comment.
Reviewing other languages bindings, I think you're right it makes more sense to throw:
| /// - Parameter waitTimeout: The amount of time to wait for element existence. Overrides the implicit wait timeout. | ||
| /// - Returns: The element that was found. | ||
| @discardableResult // for use as an assertion | ||
| public func requireElement(locator: ElementLocator, description: String? = nil, waitTimeout: TimeInterval? = nil) throws -> Element { |
There was a problem hiding this comment.
Would this be better as an extension method in our app instead? @jeffdav . I feel like it's not quite at the right layer here and I am leaning towards that (so we can have both expectElement and expectNoElement with different timeouts).
There was a problem hiding this comment.
I was going to ask, actually, because I was thinking about how to put it in the "supported apis" documentation. Could go either way. We could also have a new target in this project that is "bonus stuff".
There was a problem hiding this comment.
I don't like (nonmonetary) bonuses. I think I'll move this to the arc repo. It'll also make more sense with the custom error type.
New approach from #157 : Other Selenium bindings (Java, C#) have a throwing `findElement` method, so do the same for Swift. Also refactors `poll` to remove the `PollResult` type in favor of having the closure return a `Result<Value, Error>`. A returned error allows a retry whereas a thrown error stops the polling.
Session/Element.findElementis an outlier in that it is the only method that converts a specific error (element not found) into a nil value. It is also prone to misuse since it makes it perfectly sensible to test forfindElement(...) == nil, without realizing that this incurs the worst-case implicit wait, which may not be desirable for such negative tests.In contrast,
requireElementthrows upon failure, which makes it directly usable in positive tests (where the element should exist), with the best-case implicit wait and without having to unwrap the result.Since a pattern is to call
findElementin anXCTUnwrapand provide a description of the item that was not found,requireElementtakes an optional description and always throws the same error type which captures all relevant information for diagnostics.Alternate names considered:
expectElement(sounds too much like a test framework)getElement(too subtly different fromfindElement, could still imply a nullable return)findElement, and make the other onetryFindElement(too C#-y)Possible follow-ups: make the
waitTimeoutparameter offindElementnon-optional to call out the waiting pitfall when used in negative tests.