Stabilize FunctionalJavascript testing AJAX: add ::assertExpectedAjaxRequest()

MR back to green.

Looking at my earlier review point that ended up stalling this:

What if we added an $expected_count parameter here instead of adding the int return?

Totally possible, but … it'd be more disruptive: any contrib/custom subclass of ::assertWaitOnAjaxRequest() would need to be updated. More importantly still, you'd need to pass in an explicit timeout and message before you could pass that third new parameter … which seems pretty awful and verbose

@Wim is correct. The only thing I can think of is adding this as the first parameter to assertWaitOnAjaxRequest() - either add a new method with the new signature like assertAjaxRequestCount() or change the existing API, which would mean trying to divine whether an integer is a count or a timeout value and issuing deprecations from there etc.

Option one just seems terrible, I'd go for option 2 that way we unlock the situation with the return and have a path forward

+1 to option 2. Overloading the existing API doesn't feel like a good idea as there is no clean way to do it and the method name doesn't imply that it can also count requests so discoverability of this is hard. Adding a new method explicitly for counting the requests as well makes sense to me.

I agree with @nod_ +1 for option 2 as a way forward.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch 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.

all good on the js side for me, thanks

I've added a new method, public function assertExpectedAjaxRequest(?int $count = NULL, $timeout = 10000, $message = 'Unable to complete AJAX request.'): void {

This optionally expects a count, and will run the assertion internally if one is provided, otherwise it's exactly the same as the old method.

https://git.drupalcode.org/project/drupal/-/merge_requests/5367/diffs?co... shows the change. I also updated FrameworkTest.

If this looks OK, I can update all of the calls here which are doing assertSame() on the return value.

We could also entirely deprecate the old method and move everything over - don't particularly mind doing that if it helps this land smoothly, but it'd also be a straightforward follow-up.

Green again.

I opened 📌 Deprecate ::assertWaitOnAjaxRequest() in favour of ::assertExpectedAjaxRequest() Active .

Still some random test failures from #3317520: [PP-1] Random failure in Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testEditModeEnableDisable → but that is not ajax waits and already has its own issue.

All looks good to me

Looks really good. 🤩

Posted one question. But that could be a follow-up too — it's so clear that this is such a huge improvement to functional JS test reliability thanks to:

   * @throws \LogicException
   *   When an unnecessary invocation to this method is detected.

👆 Gone will be the false sense of security of having written a test, when actually that test is incorrect at worst or undeterministic at best!

Note: this will impact contrib, this is a theoretical BC break. Some contrib tests will stop working, because they'll run into this exception. But … that'll actually be a good thing, because each of those will be for a wrong "AJAX wait"! They'll be forced to fix it, and they (and d.o's test infra) will be better for it 👍 Tagging and because neither the API change is listed nor do we have a release note ready for module maintainers.

Can we stop this from breaking contrib and custom tests unless they are doing deprecation testing?

I.e. can we only throw the new error if the test is a core test. And if not trigger a deprecation or something. If someone has a large JS test suite that is running fine on their infrastructure causing it to fail and requiring them to remediate immediately feels not great.

Setting to needs work for #97

I.e. can we only throw the new error if the test is a core test. And if not trigger a deprecation or something. If someone has a large JS test suite that is running fine on their infrastructure causing it to fail and requiring them to remediate immediately feels not great.

Yes, using something like \Drupal\Core\Config\Schema\SchemaCheckTrait::isContribViolation():

  /**
   * Whether the current test is for a contrib module.
   *
   * @return bool
   */
  private function isContribViolation(): bool {
    $test_file_name = (new \ReflectionClass($this))->getFileName();
    $root = dirname(__DIR__, 6);
    return !str_starts_with($test_file_name, $root . DIRECTORY_SEPARATOR . 'core');
  }

I.e. can we only throw the new error if the test is a core test. And if not trigger a deprecation or something.

This is a docs issue - the code triggers a deprecation, the phpdoc appears to have been pre-emptive (or maybe a holdover from before this triggered a deprecation.

I see — my bad! 😅

Updated the IS and the CR.

Removing the 'needs release note' tag since I think the change record is enough here.

Above wasn't true for the RuntimeException so I have made that conditional on $count being passed in, we can change that issue a deprecation when $count isn't passed in later.

Latest run has a fail that looks relevant:

Drupal\FunctionalJavascriptTests\AjaxWaitTest::testUntrackedXhr
Failed asserting that exception message 'Unable to complete AJAX request.' contains '0 XHR requests through jQuery, but 1 observed in the browser — this requires js_testing_ajax_request_test.js to be updated.'.

Rerunning the pipeline but this is one of the new tests in AjaxWaitTest getting the opposite error message.

AjaxWaitTest needed updating to use ::assertExpectedAjaxRequest() with a $count parameter, otherwise the error message it's looking for doesn't get triggered (due to tweaks here to make it more bc-friendly and not trigger the new exceptions). Pushed a commit that does that.

Since the fix was just converting the test to use the new API with the stricter behaviour, after relaxing the behaviour for the old API to maintain bc, moving back to RTBC.

lauriii → closed merge request !5367

Committed 2a95e3f and pushed to 11.x. Also cherry-picked to 10.2.x. Thanks!

This has caused \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testBlocks() to fail locally 100% of the time and really really often on gitlabci.

See https://git.drupalcode.org/project/drupal/-/pipelines/51990 for example.

I got the same last week too. For example: https://git.drupalcode.org/project/drupal/-/merge_requests/5451/pipelines where the only things that were changed in that MR were "test-only" job code. I tried re-running some of the jobs multiple times and kept getting fails.

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

Just FYI for anyone else that finds this issue because their assertWaitOnAjaxRequest() calls started failing, you need to now enable js_testing_ajax_request_test for any JS test that uses this function.

js_testing_ajax_request_test provides test JS code that set window.drupalActiveXhrCount and window.drupalCumulativeXhrCount. Those variables are required to have a successful assertWaitOnAjaxRequest call.

Addition to previous comment. You don't need to add js_testing_ajax_request_test if you extends WebDriverTestBase.

Many contrib modules affected by this change:

• pathauto: 🐛 Fix phpunit erros on Drupal 10.2 Needs review
• address: 🐛 Fix functional javascript tests for Drupal 10.2 Active

And in most cases nobody understand the reason of the fail.
Changing from assertWaitOnAjaxRequest() to assertExpectedAjaxRequest() doesn't help at all.
I would love to have instruction how to handle common fail cases, e.g:
RuntimeException: Unable to complete AJAX request.

I'm confused by this change... Commerce & Commerce shipping tests are affected as well... Even though the failing tests extend the CommerceWebDriverTestBase class (which extends the WebDriverTestBase).
What am I missing? Do we need to not rely on assertWaitOnAjaxRequest() anymore?

assertWaitOnAjaxRequest() now fails if during the wait window.drupalCumulativeXhrCount never exceeds 0, so any uses of assertWaitOnAjaxRequest() that weren't actually needed will now fail.

So for tests that were previously passing and now fail on assertWaitOnAjaxRequest(), the first thing to try is removing the call entirely. It was very common to add these calls in places where they were not needed, where they were used for waiting on a JS operation to complete that wasn't necessarily something that used the Drupal AJAX API.

It is possible removing assertWaitOnAjaxRequest() will result a different test failure. Calling assertWaitOnAjaxRequest() when no Ajax calls are present still added a 10000ms wait before proceeding, and tests may have depended on that wait. Ideally, if the assertWaitOnAjaxRequest() removal results in a fail, it should be replaced with a wait for the specific expected change.

One of the reasons for this change was due to every unnecessary assertWaitOnAjaxRequest() adding 10000ms per-call to each test run. The more of these removed, the faster you can expect your tests to run.

@bnjmnm: Thanks a lot for this thorough explanation! This is helpful :).

Is this going to land in 10.3.x?

@jordan.jamous it is in 10.3.0.