Skip to content

Commit

Permalink
Fix random error in with-driver-tests (#681)
Browse files Browse the repository at this point in the history 
* Fix -connect-driver to wait for ready driver and recreate session

There is a race condition in Firefox that sometimes results in a
zombie session. When we detect this condition, we throw away the bad
session and create another one.

* Fix fake driver to wrap data in :value and add Get Window Handle

* Fix tests in proc_test.clj

* Fix tests in unit_test.clj

* Update CHANGELOG

* Fix PR comment items

* Fix OBOB and make log message and var names more accurate
  • Loading branch information
@dgr
dgr authored Oct 8, 2024
1 parent 371a394 commit b4f93eb
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 21 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.adoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ A release with an intentional breaking changes is marked with:
== Unreleased

* Changes
** {issue}676[#676]: Fix new driver creation so that it sidesteps some underlying Firefox race conditions and improves CI test stability. ({person}dgr[@dgr])
** {issue}679[#679]: Add `new-window` function that exposes WebDriver's New Window endpoint. ({person}dgr[@dgr])

== v1.1.42 - 2024-09-27 [[v1.1.42]]
Expand Down
15 changes: 12 additions & 3 deletions script/fake_driver.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,23 @@
(defn make-handler [_opts]
(fn handle-request [{:keys [request-method uri] :as _req}]
(cond
;; Get Status
(and (= :get request-method) (= "/status" uri))
{:status 200
:headers {"Content-Type" "application/json"}
:body (json/generate-string {:value {:ready true
:message "I'm ready"}})}
;; Create Session
(and (= :post request-method) (= "/session" uri))
{:status 200
:headers {"Content-Type" "application/json"}
:body (json/generate-string {:sessionId (random-uuid)})}
(and (= :get request-method) (= "/status" uri))
:body (json/generate-string {:value {:sessionId (random-uuid)}})}
;; Get Window Handle
(and (= :get request-method) (re-matches #"/session/[^/]+/window" uri))
{:status 200
:headers {"Content-Type" "application/json"}
:body (json/generate-string {:ready true})}
:body (json/generate-string {:value (random-uuid)})}
;; Fake Driver is... well... fake.
:else
{:status 404})))

Expand Down
38 changes: 36 additions & 2 deletions src/etaoin/api.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3465,6 +3465,7 @@
(:post-run-actions driver))
(recur (inc try-num) (:exception res)))))))))


(defn- -connect-driver
"Connects to a running Webdriver server.
Expand Down Expand Up @@ -3547,8 +3548,41 @@
(drv/set-capabilities (get-in defaults [type :capabilities]))
(drv/set-capabilities capabilities)))
caps (:capabilities driver)
session (create-session driver caps)]
(assoc driver :session session)))
max-tries 3]
(loop [n 1]
;; Wait for driver to be ready before creating the first session.
(wait-predicate
(fn [] (-> (get-status driver) :ready))
{:timeout 30
:interval 0.100
:message "Timeout waiting for WebDriver to become ready after creation."})
;; The following code works around a bug in
;; geckodriver/Firefox. In some cases, calling create-session on
;; a geckodriver will return a "bad" session instance. That
;; session instance will throw errors for the simplest queries.
;; The root problem appears to be a race condition somewhere in
;; the geckodriver/marionette/Firefox pathway as it is
;; inconsistent and difficult to reproduce. To work around this,
;; we try to detect a "bad session" and if we have one, we
;; delete it and create another. Note that the problem is
;; geckodriver/Firefox-specific, but we do this for all drivers
;; because it works anyway (non-Firefox drivers just return
;; a "good session") and it simplifies the logic.
(let [driver (assoc driver :session (create-session driver caps))
[good-session e] (try (get-window-handle driver)
[true nil]
(catch Exception e
[false e]))]
(if good-session
driver
(do
(delete-session driver)
(if (<= n max-tries) ; max total tries = 3
(do (log/warnf e "Bad session detected. Try again %d/%d." (inc n) max-tries)
(recur (inc n)))
(throw+ {:type :etaoin/retries
:message "Could not create a good session after multiple retries"}
e))))))))

(defn disconnect-driver
"Returns new `driver` after disconnecting from a running WebDriver process.
Expand Down
42 changes: 26 additions & 16 deletions test/etaoin/unit/proc_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,23 +105,30 @@

(deftest http-error-on-create-proc-alive
;; when etaoin tries to create a session return an http error
(with-redefs [client/http-request (fn [_] {:status 400})]
(let [ex (try
(e/with-firefox _driver
(is false "should not reach here"))
(catch Throwable ex
{:exception ex}))
exd (-> ex :exception ex-data)]
(is (= :etaoin/http-error (:type exd)))
(is (= 400 (:status exd)))
(is (= nil (-> exd :driver :process :exit)))
(is (= 0 (get-count-firefoxdriver-instances))))))
(let [orig-http-request client/http-request]
(with-redefs [client/http-request (fn [{:keys [method uri] :as params}]
;; allow get status and create session through, fail on everything else
(if (and (= :get method) (str/ends-with? uri "/status"))
(orig-http-request params)
{:status 400}))]
(let [ex (try
(e/with-firefox _driver
(is false "should not reach here"))
(catch Throwable ex
{:exception ex}))
exd (-> ex :exception ex-data)]
(is (= :etaoin/http-error (:type exd)))
(is (= 400 (:status exd)))
(is (= nil (-> exd :driver :process :exit)))
(is (= 0 (get-count-firefoxdriver-instances)))))))

(deftest http-exception-after-create-proc-now-dead
(let [orig-http-request client/http-request]
(with-redefs [client/http-request (fn [{:keys [method uri] :as params}]
;; allow create session through, fail on everything else
(if (and (= :post method) (str/ends-with? uri "/session"))
;; allow get status and create session through, fail on everything else
(if (or (and (= :get method) (str/ends-with? uri "/status"))
(and (= :post method) (str/ends-with? uri "/session"))
(and (= :get method) (re-find #"/session/[^/]+/window" uri)))
(orig-http-request params)
(throw (ex-info "read timeout" {}))))]
(let [ex (try
Expand All @@ -130,7 +137,8 @@
(is (= 1 (get-count-firefoxdriver-instances)))
(proc/kill (:process driver))
;; we'll now fail on this call
(e/go driver "https://clojure.org"))
(e/go driver "https://clojure.org")
(is false "Should have thrown an exception."))
(catch Throwable ex
{:exception ex}))
exd (-> ex :exception ex-data)]
Expand All @@ -143,8 +151,10 @@
;; unlikely, we know we just talked to the driver because it returned an http error, but for completeness
(let [orig-http-request client/http-request]
(with-redefs [client/http-request (fn [{:keys [method uri] :as params}]
;; allow create session through, fail on everything else
(if (and (= :post method) (str/ends-with? uri "/session"))
;; allow get status and create session through, fail on everything else
(if (or (and (= :get method) (str/ends-with? uri "/status"))
(and (= :post method) (str/ends-with? uri "/session"))
(and (= :get method) (re-find #"/session/[^/]+/window" uri)))
(orig-http-request params)
{:status 418}))]
(let [ex (try
Expand Down
4 changes: 4 additions & 0 deletions test/etaoin/unit/unit_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
(with-redefs
[etaoin.impl.proc/run (fn [_ _] {:some :process})
e/wait-running identity
e/get-status (fn [_] {:ready true :message "I'm ready"})
e/create-session (fn [_ _] "session-key")
e/get-window-handle (fn [_] "ABCDEFG")
proc/kill identity
e/delete-session identity
util/get-free-port (constantly 12345)]
Expand Down Expand Up @@ -142,7 +144,9 @@
[etaoin.impl.proc/run (fn [_ _]
(swap! run-calls inc)
{:some :process})
e/get-status (fn [_] {:ready true :message "I'm ready"})
e/create-session (fn [_ _] "session-key")
e/get-window-handle (fn [_] "ABCDEFG")
proc/kill (fn [_]
(swap! kill-calls inc))
e/delete-session identity
Expand Down

0 comments on commit b4f93eb

Please sign in to comment.