From 1de58c3c55006cf4b8e9705eab00ef54ab9deafd Mon Sep 17 00:00:00 2001 From: Lee Read Date: Tue, 14 May 2024 18:18:09 -0400 Subject: [PATCH] Remove chrome's `--no-sandbox` from tests (#576) Our tests included the `--no-sandbox` option for chrome. This seems to have been harmless until recently. On Windows, it now results in orphaned chrome processes which results in tests bogging and then, after a long, while failing. Removing `--no-sandbox` when running on Windows resolves the issue. The `--no-sandbox` option allows testing under a root user. Because we don't run tests under root on any OS or environment, I feel comfortable removing `--no-sandbox` across all OS testing. This change also includes new bb task `ps`, a bare-bones cross-platform way to report on running processes. This was useful while diagnosing the issue so I've left it in. I also switched from `babashka.process/destroy` to `babashka.process/destroy-tree` when killing a web driver process. This did not help with the Windows chrome orphan issue, but I expect it is generally a better way to go. Closes #572 --- CHANGELOG.adoc | 2 ++ bb.edn | 9 +++++++++ doc/01-user-guide.adoc | 2 +- script/docker_install.clj | 6 +++--- script/drivers.clj | 19 ++++--------------- script/helper/ps.clj | 18 ++++++++++++++++++ src/etaoin/ide/main.clj | 4 ++-- src/etaoin/impl/proc.cljc | 2 +- test/etaoin/api_test.clj | 2 +- test/etaoin/ide_test.clj | 2 +- test/etaoin/unit/proc_test.clj | 9 ++++----- test/etaoin/unit/unit_test.clj | 2 +- 12 files changed, 47 insertions(+), 30 deletions(-) create mode 100644 script/helper/ps.clj diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 3e6bb569..270545e1 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -11,6 +11,8 @@ A release with an intentional breaking changes is marked with: (https://github.com/tupini07[@tupini07]) * https://github.com/clj-commons/etaoin/issues/566[#566]: Recognize `:driver-log-level` for Edge * bump all deps to current versions +* tests +** https://github.com/clj-commons/etaoin/issues/572[#572]: stop using chrome `--no-sandbox` option, it has become problematic on Windows (and we did not need it anyway) * docs ** https://github.com/clj-commons/etaoin/issues/534[#534]: better describe `etaoin.api/select` and its alternatives ** https://github.com/clj-commons/etaoin/issues/536[#536]: user guide examples are now all os agnostic and CI tested via test-doc-blocks on all supported OSes diff --git a/bb.edn b/bb.edn index 6da23a45..d80816a3 100644 --- a/bb.edn +++ b/bb.edn @@ -58,6 +58,15 @@ :task test-matrix/-main} drivers {:doc "[list|kill] any running WebDrivers" :task drivers/-main} + ps {:doc "List processes with matching names (handy for debugging)" + :requires ([helper.ps :as ps] + [doric.core :as doric] + [clojure.string :as str]) + :task (let [pattern (re-pattern (str/join "|" *command-line-args*))] + (->> (ps/all-processes) + (filterv (fn [p] (re-find pattern (:command p)))) + (doric/table [:pid :start-instant :is-alive :command :arguments]) + println))} lint {:doc "[--rebuild] lint source code" :task lint/-main} cljdoc-preview {:doc "preview what docs will look like on cljdoc, use --help for args" diff --git a/doc/01-user-guide.adoc b/doc/01-user-guide.adoc index 1e290284..4b220a98 100644 --- a/doc/01-user-guide.adoc +++ b/doc/01-user-guide.adoc @@ -2850,7 +2850,7 @@ Here is a `clojure` example: [source,shell] ---- -clojure -M -m etaoin.ide.main -d firefox -p '{:port 8888 :args ["--no-sandbox"]}' -r ide/test.side +clojure -M -m etaoin.ide.main -d firefox -p '{:port 8888}' -r ide/test.side ---- As well as from an uberjar. diff --git a/script/docker_install.clj b/script/docker_install.clj index 7e841dcb..7b10e847 100644 --- a/script/docker_install.clj +++ b/script/docker_install.clj @@ -45,10 +45,10 @@ (defn- wrap-chrome "The Selenium chrome docker image wraps the chrome launcher, so I'm going with the flow here. - I don't fully understand if --no-sandbox is required for docker images, but if Selenium is doing it, - I'm not interested in figuring out if we don't need to as well. + I think the --no-sandbox option is required when running as a root user which is often the case + for docker images. - They also do the umask thing... so mimicing that as well." + Selenium also do the umask thing... so mimicing that as well." [] (status/line :head "Wrapping chrome launcher") (let [launcher (-> (shell/command {:out :string} diff --git a/script/drivers.clj b/script/drivers.clj index 073e7368..6193e72f 100644 --- a/script/drivers.clj +++ b/script/drivers.clj @@ -1,11 +1,10 @@ (ns drivers (:require [babashka.fs :as fs] [doric.core :as doric] + [helper.ps :as ps] [helper.os :as os] [helper.main :as main] - [lread.status-line :as status]) - ;; noice! bb is JDK 11 so we have ProcessHandle - (:import (java.lang ProcessHandle))) + [lread.status-line :as status])) (def web-drivers [{:name "Chrome" :bin "chromedriver"} @@ -14,19 +13,8 @@ {:name "Safari" :bin "safaridriver"} {:name "PhantomJS" :bin "phantomjs"}]) -(defn all-processes [] - (for [p (-> (ProcessHandle/allProcesses) .iterator iterator-seq) - :when (some-> p .info .command .isPresent) - :let [info (.info p) - command (-> info .command .get) - arguments (when (-> info .arguments .isPresent) - (->> info .arguments .get (into [])))]] - {:handle p - :command command - :arguments arguments})) - (defn driver-processes [] - (->> (all-processes) + (->> (ps/all-processes) (keep (fn [p] (let [pfname (-> p :command fs/file-name) pfname (if (= :win (os/get-os)) @@ -52,6 +40,7 @@ (defn report[processes] (->> processes (doric/table (keep identity [{:name :name :title "WebDriver"} + :pid :command ;; arguments are don't seem to populate on Windows (when (not= :win (os/get-os)) :arguments)])) diff --git a/script/helper/ps.clj b/script/helper/ps.clj new file mode 100644 index 00000000..2be2c8db --- /dev/null +++ b/script/helper/ps.clj @@ -0,0 +1,18 @@ +(ns helper.ps + ;; noice! bb uses a modern JDK so we have ProcessHandle + (:import (java.lang ProcessHandle))) + +(defn all-processes [] + (for [p (-> (ProcessHandle/allProcesses) .iterator iterator-seq) + :when (some-> p .info .command .isPresent) + :let [info (.info p) + command (-> info .command .get) + arguments (when (-> info .arguments .isPresent) + (->> info .arguments .get (into []))) + start-instant (-> info .startInstant .get)]] + {:pid (.pid p) + :is-alive (.isAlive p) + :start-instant start-instant + :handle p + :command command + :arguments arguments})) diff --git a/src/etaoin/ide/main.clj b/src/etaoin/ide/main.clj index 7c2a0d6d..39207941 100644 --- a/src/etaoin/ide/main.clj +++ b/src/etaoin/ide/main.clj @@ -4,7 +4,7 @@ Example: ```shell - clojure -M -m etaoin.ide.main -d firefox -p '{:port 8888 :args [\"--no-sandbox\"]}' -f /path/to/script.side + clojure -M -m etaoin.ide.main -d firefox -p '{:port 8888}' -f /path/to/script.side ``` See the [User Guide](/doc/01-user-guide.adoc#selenium-ide-cli) for more info. @@ -63,7 +63,7 @@ This is a CLI interface for running Selenium IDE files. Usage examples: ;; from clojure -clojure -M -m etaoin.ide.main -d firefox -p '{:port 8888 :args [\"--no-sandbox\"]}' -r ide/test.side +clojure -M -m etaoin.ide.main -d firefox -p '{:port 8888}' -r ide/test.side ;; from a jar java -cp .../poject.jar -m etaoin.ide.main -d firefox -p '{:port 8888}' -f ide/test.side diff --git a/src/etaoin/impl/proc.cljc b/src/etaoin/impl/proc.cljc index afe4844c..53d5c45d 100644 --- a/src/etaoin/impl/proc.cljc +++ b/src/etaoin/impl/proc.cljc @@ -49,7 +49,7 @@ For driver installation, check out the Etaoin user guide: %s" binary user-guide- (defn kill "Ask `p` to die. Use [[result]] to get exit code if you need it." [p] - (p/destroy p) + (p/destroy-tree p) @p) (defn result diff --git a/test/etaoin/api_test.clj b/test/etaoin/api_test.clj index f8d0ead4..cf8dec77 100644 --- a/test/etaoin/api_test.clj +++ b/test/etaoin/api_test.clj @@ -40,7 +40,7 @@ [:firefox :chrome :safari]) (def default-opts - {:chrome {:args ["--no-sandbox"]} + {:chrome {} :firefox {} :safari {} :edge {:args ["--headless"]}}) diff --git a/test/etaoin/ide_test.clj b/test/etaoin/ide_test.clj index 6ac6393e..b7815aed 100644 --- a/test/etaoin/ide_test.clj +++ b/test/etaoin/ide_test.clj @@ -31,7 +31,7 @@ (let [base-url (-> "static" io/resource str) test-file-path (-> "ide/test.side" io/resource str)] (doseq [type drivers] - (e/with-driver type {:args ["--no-sandbox"]} driver + (e/with-driver type driver (e/go driver base-url) (binding [*driver* driver *base-url* base-url diff --git a/test/etaoin/unit/proc_test.clj b/test/etaoin/unit/proc_test.clj index 261e21da..4d7aa6a4 100644 --- a/test/etaoin/unit/proc_test.clj +++ b/test/etaoin/unit/proc_test.clj @@ -56,7 +56,7 @@ process (proc/run ["chromedriver" (format "--port=%d" port)])] (try (e/wait-running {:port port :host "localhost"}) - (e/with-chrome {:args ["--no-sandbox"]} driver + (e/with-chrome driver ;; added to diagnose flakyness on windows on CI (println "automatically chosen port->" (:port driver)) ;; added to diagnose flakyness on windows on CI @@ -71,7 +71,7 @@ (try (e/wait-running {:port port :host "localhost"}) ;; should connect, not launch - (let [driver (e/chrome {:host "localhost" :port port :args ["--no-sandbox"]})] + (let [driver (e/chrome {:host "localhost" :port port})] (is (= 1 (get-count-chromedriver-instances))) (e/quit driver)) (finally @@ -83,7 +83,7 @@ (try (e/wait-running {:port port :host "localhost"}) (let [;; should connect, not launch - driver (e/chrome {:webdriver-url (format "http://localhost:%d" port) :args ["--no-sandbox"]})] + driver (e/chrome {:webdriver-url (format "http://localhost:%d" port)})] (is (= 1 (get-count-chromedriver-instances))) @@ -172,8 +172,7 @@ (with-redefs [client/http-request (fn [_] (throw (ex-info "read timeout" {})))] ;; attempt connect, not launch (let [ex (try - (e/with-chrome {:webdriver-url (format "http://localhost:%d" port) - :args ["--no-sandbox"]} _driver + (e/with-chrome {:webdriver-url (format "http://localhost:%d" port)} _driver (is false "should not get here")) (catch Throwable ex {:exception ex})) diff --git a/test/etaoin/unit/unit_test.clj b/test/etaoin/unit/unit_test.clj index 28f36262..618922ca 100644 --- a/test/etaoin/unit/unit_test.clj +++ b/test/etaoin/unit/unit_test.clj @@ -96,7 +96,7 @@ (deftest test-chrome-profile (fs/with-temp-dir [chrome-dir {:prefix "chrome-dir"}] (let [profile-path (str (fs/file chrome-dir "chrome-profile"))] - (e/with-chrome {:profile profile-path :args ["--no-sandbox"]} driver + (e/with-chrome {:profile profile-path} driver (e/go driver "chrome://version") (is profile-path (e/get-element-text driver :profile_path))))))