Skip to content

Commit

Permalink
Remove chrome's --no-sandbox from tests
Browse files Browse the repository at this point in the history
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
  • Loading branch information
lread committed May 14, 2024
1 parent 6864d92 commit f6da6a1
Show file tree
Hide file tree
Showing 12 changed files with 47 additions and 30 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 9 additions & 0 deletions bb.edn
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion doc/01-user-guide.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions script/docker_install.clj
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down
19 changes: 4 additions & 15 deletions script/drivers.clj
Original file line number Diff line number Diff line change
@@ -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"}
Expand All @@ -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))
Expand All @@ -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)]))
Expand Down
18 changes: 18 additions & 0 deletions script/helper/ps.clj
Original file line number Diff line number Diff line change
@@ -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}))
4 changes: 2 additions & 2 deletions src/etaoin/ide/main.clj
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/etaoin/impl/proc.cljc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion test/etaoin/api_test.clj
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
[:firefox :chrome :safari])

(def default-opts
{:chrome {:args ["--no-sandbox"]}
{:chrome {}
:firefox {}
:safari {}
:edge {:args ["--headless"]}})
Expand Down
2 changes: 1 addition & 1 deletion test/etaoin/ide_test.clj
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 4 additions & 5 deletions test/etaoin/unit/proc_test.clj
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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)))
Expand Down Expand Up @@ -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}))
Expand Down
2 changes: 1 addition & 1 deletion test/etaoin/unit/unit_test.clj
Original file line number Diff line number Diff line change
Expand Up @@ -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))))))
Expand Down

0 comments on commit f6da6a1

Please sign in to comment.