Add wait-url-change macro to make tests more reliable #675
Conversation
This macro uses wait-predicate to poll the WebDriver for a change in the URL, thus eliminating some race conditions that were causing flaky test failures.
In addition to waiting for the URL to change, wait-url-change also waits for the new URL to match a regex (with re-find).
|
This passed everything except for the other race condition in another part of the test suite. I create an issue for that other race condition. I think you can merge this PR now and we can work on the other issue later. |
| :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"))) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So, a few things:
- The wait for change is actually a good self-check. There were a few tests in the suite that I discovered had the previous
testingblock 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'sabout:blanknonsense, then that's good enough to say that we're ready to start making test assertions" strategy. - 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.
- It doesn't have an actual
clojure.testassertion 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.
There was a problem hiding this comment.
Just checking if my poh-tay-toe might pique your interest at all.
Your poh-tah-toe is fine with me.
Closes #646
Closes #674
This macro uses wait-predicate to poll the WebDriver for a change in the URL, thus eliminating some race conditions that were causing flaky test failures.
Please complete and include the following checklist:
I have read CONTRIBUTING and the Etaoin Developer Guide.
This PR corresponds to an issue that the Etaoin maintainers have agreed to address.
This PR contains test(s) to protect against future regressions
I have updated CHANGELOG.adoc with a description of the addressed issue.