Skip to content

Switch to querySelector #111

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

Closed
uuf6429 wants to merge 2 commits into from
Closed

Switch to querySelector #111

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
f39b229
Switch to querySelector
uuf6429 Nov 24, 2025
d79b9f0
Add jetbrains attrib dev package
uuf6429 Nov 24, 2025
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
3 changes: 2 additions & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@
"phpstan/phpstan": "^1.10",
"phpstan/phpstan-phpunit": "^1.3",
"phpstan/phpstan-symfony": "^1.3",
"symfony/http-kernel": "^4.4 || ^5.0 || ^6.0 || ^7.0 || ^8.0"
"symfony/http-kernel": "^4.4 || ^5.0 || ^6.0 || ^7.0 || ^8.0",
"jetbrains/phpstorm-attributes": "^1.2"
},
"conflict": {
"symfony/http-foundation": "<4.4 || >=9",
Expand Down
12 changes: 6 additions & 6 deletions tests/Form/GeneralTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public function testBasicForm(): void

$page->pressButton('Save');

if ($this->safePageWait(5000, 'document.getElementById("first") !== null')) {
if ($this->safePageWait(5000, 'document.querySelector("#first") !== null')) {
Copy link
Member

@aik099 aik099 Nov 25, 2025

Choose a reason for hiding this comment

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

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.querySelector method would return the DOM element from the old page;
  • the document.getElementById would 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)

Copy link
Member Author

@uuf6429 uuf6429 Nov 25, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@aik099 aik099 Nov 25, 2025

Choose a reason for hiding this comment

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

Then you're not getting 5sec delay each time and querySelector works as expected?

Copy link
Member Author

@uuf6429 uuf6429 Nov 25, 2025

Choose a reason for hiding this comment

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

looks like it (the ones with the red mark have the 5s wait, but test finished quickly):
image

Copy link
Member Author

@uuf6429 uuf6429 Nov 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

$this->assertEquals('Anket for Konstantin', $webAssert->elementExists('css', 'h1')->getText());
$this->assertEquals('Firstname: Konstantin', $webAssert->elementExists('css', '#first')->getText());
$this->assertEquals('Lastname: Kudryashov', $webAssert->elementExists('css', '#last')->getText());
Expand All @@ -70,7 +70,7 @@ public function testFormSubmitWays(string $submitVia): void

$page->pressButton($submitVia);

if ($this->safePageWait(5000, 'document.getElementById("first") !== null')) {
if ($this->safePageWait(5000, 'document.querySelector("#first") !== null')) {
$this->assertEquals('Firstname: Konstantin', $webAssert->elementExists('css', '#first')->getText());
} else {
$this->fail('Form was never submitted');
Expand Down Expand Up @@ -98,7 +98,7 @@ public function testFormSubmit(): void

$webAssert->elementExists('xpath', 'descendant-or-self::form[1]')->submit();

if ($this->safePageWait(5000, 'document.getElementById("first") !== null')) {
if ($this->safePageWait(5000, 'document.querySelector("#first") !== null')) {
$this->assertEquals('Firstname: Konstantin', $webAssert->elementExists('css', '#first')->getText());
}
}
Expand All @@ -113,7 +113,7 @@ public function testFormSubmitWithoutButton(): void

$webAssert->elementExists('xpath', 'descendant-or-self::form[1]')->submit();

if ($this->safePageWait(5000, 'document.getElementById("first") !== null')) {
if ($this->safePageWait(5000, 'document.querySelector("#first") !== null')) {
$this->assertEquals('Firstname: Konstantin', $webAssert->elementExists('css', '#first')->getText());
}
}
Expand Down Expand Up @@ -204,7 +204,7 @@ public function testAdvancedForm(): void

$button->press();

if ($this->safePageWait(5000, 'document.getElementsByTagName("title") === "Advanced form save"')) {
if ($this->safePageWait(5000, 'document.querySelector("title")?.textContent === "Advanced form save"')) {
$out = <<<'OUT'
array(
agreement = `on`,
Expand Down Expand Up @@ -245,7 +245,7 @@ public function testQuoteInValue(): void

$button->press();

if ($this->safePageWait(5000, 'document.getElementsByTagName("title") !== null')) {
if ($this->safePageWait(5000, 'document.querySelector("title") !== null')) {
Copy link
Member Author

@uuf6429 uuf6429 Nov 24, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Member

@aik099 aik099 Nov 25, 2025

Choose a reason for hiding this comment

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

Agreed. We should check such checks to more bullet-proof ones. See my other comment.

$out = <<<'OUT'
first_name = `Foo &quot;item&quot;`,
last_name = `Bar`,
Expand Down
10 changes: 5 additions & 5 deletions tests/Form/Html5Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public function testHtml5FormInputAttribute(): void

$page->pressButton('Submit in form');

if ($this->safePageWait(5000, 'document.getElementsByTagName("title") !== null')) {
if ($this->safePageWait(5000, 'document.querySelector("title") !== null')) {
$out = <<<'OUT'
first_name = `John`,
last_name = `Doe`,
Expand Down Expand Up @@ -72,7 +72,7 @@ public function testHtml5FormButtonAttribute(): void

$page->pressButton('Submit outside form');

if ($this->safePageWait(5000, 'document.getElementsByTagName("title") !== null')) {
if ($this->safePageWait(5000, 'document.querySelector("title") !== null')) {
$out = <<<'OUT'
first_name = `John`,
last_name = `Doe`,
Expand All @@ -91,7 +91,7 @@ public function testHtml5FormOutside(): void

$page->pressButton('Submit separate form');

if ($this->safePageWait(5000, 'document.getElementsByTagName("title") !== null')) {
if ($this->safePageWait(5000, 'document.querySelector("title") !== null')) {
$out = <<<'OUT'
other_field = `hello`,
OUT;
Expand Down Expand Up @@ -137,7 +137,7 @@ public function testHtml5FormAction(): void
$page->fillField('first_name', 'Jimmy');
$page->pressButton('Submit to basic form');

if ($this->safePageWait(5000, 'document.getElementsByTagName("title") !== null')) {
if ($this->safePageWait(5000, 'document.querySelector("title") !== null')) {
$this->assertStringContainsString('<title>Basic Form Saving</title>', $page->getContent());
$this->assertStringContainsString('Firstname: Jimmy', $page->getContent());
}
Expand All @@ -152,7 +152,7 @@ public function testHtml5FormMethod(): void
$page->fillField('last_name', 'Jones');
$page->pressButton('Submit as GET');

if ($this->safePageWait(5000, 'document.getElementsByTagName("title") !== null')) {
if ($this->safePageWait(5000, 'document.querySelector("title") !== null')) {
$this->assertEquals(
$this->pathTo('advanced_form_post.php') . '?first_name=Jimmy&last_name=Jones',
$this->getSession()->getCurrentUrl()
Expand Down
7 changes: 5 additions & 2 deletions tests/TestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,11 @@ protected function pathTo($path)
*
* @see \Behat\Mink\Session::wait()
*/
protected function safePageWait($time, $condition)
{
protected function safePageWait(
$time,
#[\JetBrains\PhpStorm\Language('JavaScript')]
$condition
) {
try {
return $this->getSession()->wait($time, $condition);
} catch (UnsupportedDriverActionException $e) {
Expand Down