diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 48e54eb..fb8a67d 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -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]] diff --git a/script/fake_driver.clj b/script/fake_driver.clj index b020dd4..44aa224 100644 --- a/script/fake_driver.clj +++ b/script/fake_driver.clj @@ -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}))) diff --git a/src/etaoin/api.clj b/src/etaoin/api.clj index add03c4..d3c4f59 100644 --- a/src/etaoin/api.clj +++ b/src/etaoin/api.clj @@ -3465,6 +3465,7 @@ (:post-run-actions driver)) (recur (inc try-num) (:exception res))))))))) + (defn- -connect-driver "Connects to a running Webdriver server. @@ -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. diff --git a/test/etaoin/unit/proc_test.clj b/test/etaoin/unit/proc_test.clj index 62f4c8e..21f6e2c 100644 --- a/test/etaoin/unit/proc_test.clj +++ b/test/etaoin/unit/proc_test.clj @@ -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 @@ -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)] @@ -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 diff --git a/test/etaoin/unit/unit_test.clj b/test/etaoin/unit/unit_test.clj index 87b2529..6621b86 100644 --- a/test/etaoin/unit/unit_test.clj +++ b/test/etaoin/unit/unit_test.clj @@ -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)] @@ -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