Skip to content
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

Fix random error in with-driver-tests #681

Merged
merged 7 commits into from
Oct 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.adoc
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
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
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.
dgr marked this conversation as resolved.
Show resolved Hide resolved
(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
dgr marked this conversation as resolved.
Show resolved Hide resolved
: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
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
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
Loading