[random test failure] Re-enable AjaxTest::testAjaxFocus()

https://git.drupalcode.org/project/drupal/-/jobs/259200

It's harder (impossible) to see branch history for core scheduled pipelines on gitlab compared to drupalci, but I'm only seeing this on sqlite so far, so I think we should skip it when the database driver is sqlite, and open an issue to debug and then re-enable.

MySQL this time https://git.drupalcode.org/issue/drupal-3398321/-/jobs/261673

SQLite: https://git.drupalcode.org/project/drupal/-/jobs/294580

kim.pepper → made their first commit to this issue’s fork.

Created 📌 Skip AjaxTest::testAjaxFocus() due to random test failures Needs review

Bumping to critical to re-enable now the skipping is in.

We should try an MR here to unskip this test once 📌 Stabilize FunctionalJavascript testing AJAX: add ::assertExpectedAjaxRequest() Fixed is in.

Gitlab allows repeating the test class multiple times, so can use that to see if we still get random test failures or not.

Tagging novice for those two bits, but if there are still random test failures in this test once 📌 Stabilize FunctionalJavascript testing AJAX: add ::assertExpectedAjaxRequest() Fixed is in, that is unfortunately not novice any more.

Seems to have some failures.

The fact this was still failing suggests this wasn't an insufficient lack of AJAX waiting, so I had a closer look, and indeed it might not be. Technically it is still the AJAX API, but we have code that runs after a successful response, which means waiting for a successful response is insufficient. Made a commit and kicked off a couple of sqlite runs since that seems to be the easiest way to reproduce this.

Well that did absolutely nothing useful.

Not sure if it was answered somewhere else but do we know why the randoms have gone up last few weeks

@smustgrave we had similar failures in the same tests on DrupalCI until a few months ago, then fixed it on there, so I'm suspecting a configuration regression on gitlab, but nothing is pinned down yet. One issue at #3397749: gitlab pod DNS settings → .

Whilst investigating the random failures and DNS request, this issue was/is one of the most common failures.

It might not be too slow, it might actually be too fast…

I refactored the AjaxTest like this.

From:

    // Test textfield with 'change' event listener with refocus-blur set to
    // FALSE.
    $textfield2->setValue('Llamas say yarhar');
    $textfield3->focus();
    $this->assertSession()->assertWaitOnAjaxRequest();
    $has_focus_id = $this->getSession()->evaluateScript('document.activeElement.id');
    $this->assertEquals('edit-textfield-2', $has_focus_id);

To:

    // Test textfield with 'change' event listener with refocus-blur set to
    // FALSE.
    $textfield2->setValue('Llamas say yarhar');
    // The above does a blur at the end, so the focus is gone.
    $has_focus_id = $this->getSession()->evaluateScript('document.activeElement.id');
    $this->assertNotEquals('edit-textfield-2', $has_focus_id);
    $this->assertSession()->assertWaitOnAjaxRequest();
    // The focus is now back after the request.
    $has_focus_id = $this->getSession()->evaluateScript('document.activeElement.id');
    $this->assertEquals('edit-textfield-2', $has_focus_id);

See specifically this line $this->assertNotEquals('edit-textfield-2', $has_focus_id);

That seems to fail way less times.

Will give more details tomorrow when I’m back to it.

I think that the test cannot be really tested without risking a random failure.
When we are setting the value of the text field, the Selenium2Driver::setValue method runs document.activeElement.blur(); right after. See here.

Running:

    $textfield2->setValue('Llamas say yarhar');
    $textfield3->focus();

Leads to three cases:
- Ajax too slow. $this->assertSession()->assertWaitOnAjaxRequest(); won't be enough and $has_focus_id = $this->getSession()->evaluateScript('document.activeElement.id'); will give us text field 3.
- Ajax too quick. The focus was already set right after calling setValue and by the time $textfield3->focus(); we completely overrode the focus value.
- Ajax takes a bit but not too much. Both the the cases in the previous comment do work for the most part, but the second one seems more robust than the first.

I'm going to create an MR based on this, but I actually think that we have no proper way of testing this (or at least I don't know of a test helper that might be useful here).

MR https://git.drupalcode.org/project/drupal/-/merge_requests/5608 has a refactoring that seems to fail less times than the current version.
My actual suggestion is to remove the block as there is no reliable way to check the change of focus if the AJAX request runs too quick.

I think it would be OK to remove this section as impossible to reliably test. We could always open a normal task to try to add something back if someone has better ideas.

I've fully removed that part of the test now.

Let's do it.

Committed and pushed e704201048 to 11.x and d9a161a89b to 10.2.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.

I'm seeing this again once or twice per day, e.g. https://git.drupalcode.org/project/drupal/-/jobs/1133184

1) Drupal\FunctionalJavascriptTests\Ajax\AjaxTest::testAjaxFocus
    Failed asserting that two strings are equal.
    --- Expected
    +++ Actual
    @@ @@
    -'edit-textfield-3'
    +'edit-textfield-2'

I think we might want to roll back the commit here and then continue in the same issue. Normally not keen on that approach, but given it's purely a test issue and the history is here, it might be better to keep things in one place.

Done the revert everywhere...

What is interesting is that if you run the test locally you see that this is completely borked even if you try to do the same steps.

It looks like the revert (i.e., commit 966fd63a6c7fd5ae9cd61222717eac2644efc872) is causing the needs-review-queue-bot to reject all issues on needs review. The problem seems to be the string yarhar. Recent example:

• https://www.drupal.org/project/drupal/issues/3401255#comment-15602961 📌 Tighten config validation schema of system.mail mailer_dsn Needs work
• https://www.drupal.org/files/issues/2024-05-20/3401255-nr-bot_0.txt →

Added a patch, I guess we'd need something like that.

So have not seen cspell be random. The tests in the issue summary I have seen before (though has been a week or so).

The cspell thing isn't random. It is reproducible. Any patch/PR which is touching the dictionary will trigger a cspell failure whenever core/scripts/dev/commit-code-check.sh (that's what the needs-review-queue-bot is doing). This is because the commit-code-check.sh conditionally runs a full cspell check:

# Check all files for spelling in one go for better performance.
  if [[ $CSPELL_DICTIONARY_FILE_CHANGED == "1" ]] ; then
    printf "\nRunning spellcheck on *all* files.\n"
    yarn run spellcheck:core --no-must-find-files --no-progress
  else
[...]

The problem is that between the initial commit (#24 from January 1) and the revert of it (#33 May 6) another 📌 Remove 14 foreign words from the dictionary Fixed removed the word yarhar from the global dictionary file. As a result, the revert (which introduced a code chunk containing that word) reintroduced a word into AjaxTest.php (see needs-review-bot output from #34).

Running spellcheck on *all* files.
./core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxTest.php:332:39 - Unknown word (yarhar)

CSpell: failed

The patch in #35 is fixing that regression.

Btw, the regression is easily reproducible by just running cspell over a fresh 11.x checkout:

% yarn run spellcheck:core --no-must-find-files --no-progress
./core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxTest.php:332:39 - Unknown word (yarhar)
CSpell: Files checked: 15180 (15179 from cache), Issues found: 1 in 1 file.

@znerol yeah you are right - sorry I missed this. It was caused by the revert and 📌 Remove 14 foreign words from the dictionary Fixed clashing. I propose a better fix in 🐛 A revert has cause cspell to fail due to the word yarhar Active

Patch in #35 is now obsolete.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

This is actually still 11.x; it was left at 10.2.x by accident after the revert.