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.

Sign up for GitHub

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

Add wait-url-change macro to make tests more reliable #675

Merged
merged 2 commits into from
Sep 24, 2024
Merged

Add wait-url-change macro to make tests more reliable #675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
18169d3
Add wait-url-change macro to make tests more reliable
dgr Sep 24, 2024
3b1e29c
Update wait-url-change to wait for URL to match a regex
dgr Sep 24, 2024
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
2 changes: 2 additions & 0 deletions CHANGELOG.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ A release with an intentional breaking changes is marked with:
** {issue}656[#656]: Correctly describe behavior when query's parameter is a string. The User Guide and `query` doc strings say that a string passed to `query` is interpreted as an XPath expression. In fact, `query` interprets this as either XPath or CSS depending on the setting of the driver's `:locator` parameter, which can be changed. ({person}dgr[@dgr])
* Quality
** {issue}640[#640]: Significantly improved test coverage. ({person}dgr[@dgr])
** {issue}646[#646]: Fixed race condition leading to unreliable test. ({person}dgr[@dgr])
** {issue}674[#674]: Fixed race condition leading to unreliable test. ({person}dgr[@dgr])

== v1.1.41 [minor breaking] - 2024-08-14 [[v1.1.41]]

Expand Down
201 changes: 92 additions & 109 deletions test/etaoin/api_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,22 @@
report-browsers
test-server)

(defn reload-test-page
[]
(e/go *driver* (test-server-url "test.html"))
(e/wait-visible *driver* {:id :document-end}))

(defmacro wait-url-change
[re & body]
`(let [old-url# (e/get-url *driver*)]
~@body
(e/wait-predicate (fn [] (let [new-url# (e/get-url *driver*)]
(and (not= old-url# new-url#)
(re-find ~re new-url#))))
{:timeout 30 ; 30 second timeout total
:interval 0.100 ; poll at 100 msec interval
:message "Timeout waiting for URL change"})))

(deftest test-browser-conditionals
(testing "Chrome conditionals"
(e/when-chrome *driver*
Expand Down Expand Up @@ -187,42 +203,28 @@
(is (e/selected? *driver* :vehicle3)))

(deftest test-submit
(doto *driver*
(e/fill-multi {:simple-input 1
:simple-password 2
:simple-textarea 3})
(e/submit :simple-input)
;; Both Safari and Firefox need a slight delay here before the URL
;; corresponding to the submitted form is valid. Chrome and Edge
;; seem to do OK without. As of Aug 28, 2024.
(e/when-safari (e/wait 3))
(e/when-firefox (e/wait 3))
(-> e/get-url
(str/ends-with? "?login=1&password=2&message=3")
is)))
(e/fill-multi *driver* {:simple-input 1
:simple-password 2
:simple-textarea 3})
(wait-url-change #"login"
(e/submit *driver* :simple-input))
(is (str/ends-with? (e/get-url *driver*) "?login=1&password=2&message=3")))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering why you bothered with a macro, but I think I see your motivation.

An alternative would be to have a wait- fn like those in Etaoin's public api.
And you'd just wait for your expected URL (no need to check if it changed) and fail via timeout exception.
But then you'd not have an assertion in your test, which might feel less natural for a test.

That's probably what I would have done, but if you are still happy with your macro after reading my comment, I'm okay with it, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, a few things:

  1. The wait for change is actually a good self-check. There were a few tests in the suite that I discovered had the previous testing block that performed a test left the browser URL state such that it was unchanged (e.g., two tests tests for the same browser URL back to back; the first passes and the second does't do anything because if the test suite is faster than the browser, it just tests the results of the previous test, not the current test). If you force a change in the URL, then it will catch mistakes like this in the test suite. That said, you could certainly remove the test for change and just look for the URL you're looking for. One issue that still has is that failure is slow. That is, if you don't get the result you're looking for, you'll have to fully timeout because you don't really know if you should continue waiting for more or not. I'd rather have all tests be fast if possible, whether passing or failing. So, that led to the "if the URL changed and it has at least enough of what I'm looking for to know that it's not Firefox's about:blank nonsense, then that's good enough to say that we're ready to start making test assertions" strategy.
  2. It's a macro because I want to detect a change in the URL, so it needs to wrap the body that triggers the change so that it can first grab the URL before the change, then execute the body to trigger, and then wait for things to change. If it was a function, you'd have to pass in a thunk that triggered the change, and I generally prefer macros to thunks; they're just more readable, IMO.
  3. It doesn't have an actual clojure.test assertion in it because I just wanted it to wait and detect a change, without making it too complicated beyond that. Once that change is detected, there may be multiple assertions that the test writer wants to run. There aren't examples of that in the test code right now, but I like composable pieces if possible.

So, poh-tay-toe / poh-tah-toe. That said, as written the test suite passed 100%, which was interesting to see.

Copy link
Collaborator

@lread lread Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking if my poh-tay-toe might pique your interest at all.
Your poh-tah-toe is fine with me.


(deftest test-input
(testing "fill multiple inputs"
;; Test with map form
(doto *driver*
(e/fill-multi {:simple-input 1
:simple-password 2
:simple-textarea 3})
(e/click :simple-submit)
(e/when-safari (e/wait 3))
(-> e/get-url
(str/ends-with? "?login=1&password=2&message=3")
is))
(e/fill-multi *driver* {:simple-input 1
:simple-password 2
:simple-textarea 3})
(wait-url-change #"login" (e/click *driver* :simple-submit))
(is (str/ends-with? (e/get-url *driver*) "?login=1&password=2&message=3"))
;; Test with vector form
(doto *driver*
(e/fill-multi [:simple-input 1
:simple-password 2
:simple-textarea 3])
(e/click :simple-submit)
(e/when-safari (e/wait 3))
(-> e/get-url
(str/ends-with? "?login=1&password=2&message=3")
is)))
(e/fill-multi *driver*
[:simple-input 4
:simple-password 5
:simple-textarea 6])
(wait-url-change #"login" (e/click *driver* :simple-submit))
(is (str/ends-with? (e/get-url *driver*) "?login=4&password=5&message=6")))
(testing "fill-multi bad inputs"
(is (thrown+? [:type :etaoin/argument]
(e/fill-multi *driver* #{:set :is :not :allowed})))
Expand All @@ -231,26 +233,20 @@
(is (thrown+? [:type :etaoin/argument]
(e/fill-multi *driver* [:vector :with :odd :length :is :not :allowed]))))
(testing "fill human multiple inputs"
(doto *driver*
;; Test with map form
(e/fill-human-multi {:simple-input "login"
:simple-password "123"
:simple-textarea "text"})
(e/click :simple-submit)
(e/when-safari (e/wait 3))
(-> e/get-url
(str/ends-with? "?login=login&password=123&message=text")
is))
(doto *driver*
;; Test with vector form
(e/fill-human-multi [:simple-input "login"
:simple-password "123"
:simple-textarea "text"])
(e/click :simple-submit)
(e/when-safari (e/wait 3))
(-> e/get-url
(str/ends-with? "?login=login&password=123&message=text")
is)))
;; Test with map form
(e/fill-human-multi *driver*
{:simple-input "login"
:simple-password "123"
:simple-textarea "text"})
(wait-url-change #"login" (e/click *driver* :simple-submit))
(is (str/ends-with? (e/get-url *driver*) "?login=login&password=123&message=text"))
;; Test with vector form
(e/fill-human-multi *driver*
[:simple-input "login2"
:simple-password "456"
:simple-textarea "text2"])
(wait-url-change #"login" (e/click *driver* :simple-submit))
(is (str/ends-with? (e/get-url *driver*) "?login=login2&password=456&message=text2")))
(testing "fill-human-multi bad inputs"
(is (thrown+? [:type :etaoin/argument]
(e/fill-multi *driver* #{:set :is :not :allowed})))
Expand All @@ -259,35 +255,27 @@
(is (thrown+? [:type :etaoin/argument]
(e/fill-multi *driver* [:vector :with :odd :length :is :not :allowed]))))
(testing "fill multiple vars"
(doto *driver*
(e/fill :simple-input 1 "test" 2 \space \A)
(e/click :simple-submit)
(e/when-safari (e/wait 3))
(-> e/get-url
(str/ends-with? "?login=1test2+A&password=&message=")
is)))
(e/fill *driver* :simple-input 1 "test" 2 \space \A)
(wait-url-change #"login" (e/click *driver* :simple-submit))
(is (str/ends-with? (e/get-url *driver*) "?login=1test2+A&password=&message=")))
(testing "fill active"
(doto *driver*
(e/click :simple-input)
(e/fill-active "MyLogin")
(e/click :simple-password)
(e/fill-active "MyPassword")
(e/click :simple-textarea)
(e/fill-active "Some text")
(e/click :simple-submit)
(e/when-safari (e/wait 3)))
(e/click *driver* :simple-input)
(e/fill-active *driver* "MyLogin")
(e/click *driver* :simple-password)
(e/fill-active *driver* "MyPassword")
(e/click *driver* :simple-textarea)
(e/fill-active *driver* "Some text")
(wait-url-change #"login" (e/click *driver* :simple-submit))
(is (str/ends-with? (e/get-url *driver*)
"?login=MyLogin&password=MyPassword&message=Some+text")))
(testing "fill active human"
(doto *driver*
(e/click :simple-input)
(e/fill-human-active "MyLogin2")
(e/click :simple-password)
(e/fill-human-active "MyPassword2")
(e/click :simple-textarea)
(e/fill-human-active "Some text 2")
(e/click :simple-submit)
(e/when-safari (e/wait 3)))
(e/click *driver* :simple-input)
(e/fill-human-active *driver* "MyLogin2")
(e/click *driver* :simple-password)
(e/fill-human-active *driver* "MyPassword2")
(e/click *driver* :simple-textarea)
(e/fill-human-active *driver* "Some text 2")
(wait-url-change #"login" (e/click *driver* :simple-submit))
(is (str/ends-with? (e/get-url *driver*)
"?login=MyLogin2&password=MyPassword2&message=Some+text+2"))))

Expand Down Expand Up @@ -324,27 +312,27 @@

(deftest test-clear
(testing "simple clear"
(doto *driver*
(e/fill {:id :simple-input} "test")
(e/clear {:id :simple-input})
(e/click {:id :simple-submit})
(e/when-safari (e/wait 3))
(-> e/get-url
(str/ends-with? "?login=&password=&message=")
is)))
(e/fill *driver* {:id :simple-input} "test")
(e/clear *driver* {:id :simple-input})
(wait-url-change #"login" (e/click *driver* {:id :simple-submit}))
(is (str/ends-with? (e/get-url *driver*) "?login=&password=&message=")))

(testing "multiple clear"
(doto *driver*
(e/fill-multi {:simple-input 1
:simple-password 2
:simple-textarea 3})
(e/clear :simple-input
:simple-password
:simple-textarea)
(e/when-safari (e/wait 3))
(-> e/get-url
(str/ends-with? "?login=&password=&message=")
is))))
;; Note that we need to reload the test page here because the URL
;; from the first test is the same as the second and thus if we
;; didn't do this we couldn't detect whether anything happened
;; after the first test.
(reload-test-page)
(e/fill-multi *driver*
{:simple-input 1
:simple-password 2
:simple-textarea 3})
(e/clear *driver*
:simple-input
:simple-password
:simple-textarea)
(wait-url-change #"login" (e/click *driver* {:id :simple-submit}))
(is (str/ends-with? (e/get-url *driver*) "?login=&password=&message="))))

(deftest test-enabled
(doto *driver*
Expand Down Expand Up @@ -427,14 +415,10 @@
(is (= inner-html result))))

(deftest test-title
(doto *driver*
(-> e/get-title (= "Webdriver Test Document") is)))
(is (= (e/get-title *driver*) "Webdriver Test Document")))

(deftest test-url
(doto *driver*
(-> e/get-url
(str/ends-with? "/test.html")
is)))
(is (str/ends-with? (e/get-url *driver*) "/test.html")))

(deftest test-css-props
(testing "single css"
Expand Down Expand Up @@ -821,12 +805,11 @@

(deftest test-set-hash
(testing "set hash"
(doto *driver*
(e/set-hash "hello")
(-> e/get-hash (= "hello") is)
(-> e/get-url (str/ends-with? "/test.html#hello") is)
(e/set-hash "goodbye")
(-> e/get-url (str/ends-with? "/test.html#goodbye") is))))
(e/set-hash *driver* "hello")
(is (= (e/get-hash *driver*) "hello"))
(is (str/ends-with? (e/get-url *driver*) "/test.html#hello"))
(e/set-hash *driver* "goodbye")
(is (str/ends-with? (e/get-url *driver*) "/test.html#goodbye"))))

(deftest test-query
(testing "finding an element by id keyword"
Expand Down Expand Up @@ -1159,8 +1142,8 @@
(e/add-pointer-click-el textarea)
e/add-pause
(e/add-pointer-click-el submit))]
(e/perform-actions *driver* keyboard mouse)
(e/wait 1)
(wait-url-change #"login"
(e/perform-actions *driver* keyboard mouse))
(is (str/ends-with? (e/get-url *driver*) "?login=1&password=2&message=3")))))

(deftest test-shadow-dom
Expand Down
Loading