Switch to querySelector #111
Conversation
| $button->press(); | ||
|
|
||
| if ($this->safePageWait(5000, 'document.getElementsByTagName("title") !== null')) { | ||
| if ($this->safePageWait(5000, 'document.querySelector("title") !== null')) { |
There was a problem hiding this comment.
I'm wondering if these cases with the title element actually makes sense - there's a pretty high chance we will always have a title element (on both the before and the after pages), so it feels kinda pointless?
There was a problem hiding this comment.
Agreed. We should check such checks to more bullet-proof ones. See my other comment.
| $page->pressButton('Save'); | ||
|
|
||
| if ($this->safePageWait(5000, 'document.getElementById("first") !== null')) { | ||
| if ($this->safePageWait(5000, 'document.querySelector("#first") !== null')) { |
There was a problem hiding this comment.
Author of the https://www.reddit.com/r/learnjavascript/comments/i0f5o8/comment/fzqhln4/ comment tells (I haven't checked that personally) that after DOM update:
- the
document.querySelectormethod would return the DOM element from the old page; - the
document.getElementByIdwould return the DOM element from the new page.
If that is really the case, then #first element won't be found at all and you'll have a 5-second delay all the time.
Have you checked this delay while running a new test suite version (from this PR) locally?
IMO we should be doing either of these:
- a bit cryptic, but working: performing id-based check (
document.getElementById) for an element that is only present on the page loaded after the form submission is finished - transparent, but might be slower: checking for target (shown after submitting) page title (must be unique across the test suite)
There was a problem hiding this comment.
I'm not sure how you are reading that, the comment seems to say something else (unrelated to the page).
From what I know, both return the same element object. This can be easily checked with document.getElementById("first") === document.querySelector("#first").
I think the misunderstanding is from getElementsByClassName (returns a live HTMLCollection) vs querySelectorAll (which returns a non-live NodeList). According to some search, there is no such thing as a live/non-live node - that term only applies to collections, and it means whether looping through the collection returns items from when the collection was produced (non-live) or re-executes the condition each time (live).
And in any case, it should be impossible for plain javascript (which we are executing within the context of a page) to retrieve elements from a previous page.
- https://developer.mozilla.org/en-US/docs/Web/API/Document/getElementById
- https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelector
- https://developer.mozilla.org/en-US/docs/Web/API/Document/getElementsByClassName
- https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelectorAll
There was a problem hiding this comment.
Then you're not getting 5sec delay each time and querySelector works as expected?
There was a problem hiding this comment.
looks like it (the ones with the red mark have the 5s wait, but test finished quickly):
There was a problem hiding this comment.
However that doesn't fully disprove your point - we are only checking if the title element exists - it doesn't check which page it is from.
| $button->press(); | ||
|
|
||
| if ($this->safePageWait(5000, 'document.getElementsByTagName("title") !== null')) { | ||
| if ($this->safePageWait(5000, 'document.querySelector("title") !== null')) { |
There was a problem hiding this comment.
Agreed. We should check such checks to more bullet-proof ones. See my other comment.
|
The whole waiting trick looks useless (we can still fix conditions used for waiting in this PR), because:
I wonder why tests in minkphp/webdriver-classic-driver#65 started to fail. |
|
I just realised, in addition safePageWait always returns true, so the conditions in the test are also useless. E.g. if ($this->safePageWait(5000, 'document.querySelector("title") !== null')) {
$out = <<<'OUT' <-- this always executes
...
} |
|
What the above reasons in mind, we should rather remove that code as @aik099 said, so I can close this PR. |
2 participants
Fixes #109.