Make the JsWebAssert::waitFor*() methods behave like real assertions

To keep the scope of this issue small, I'm not sure we should attack this piece of it yet:

Remove verifying assertions on the response from JsWebAssert::waitFor*() methods

That will make the proposed change set huge and harder to review, so maybe it's the sort of clean-up that could be done later, on an ongoing basis, in a long set of novice-level issues?

Discussed the implications of this with @phenaproxima in Slack.

There are hundreds of calls to waitForElementVisible() in contrib alone. Fixing this directly by adding assertions straight away means that contrib tests will likely break as they likely share some of the same false expectations.

We could deprecate the waitFor... methods and replace them with similar ones that assert as well, but naming these very similar new methods is hard, and it's a lot of work for contrib.

An alternative idea was to set a waitForShouldAssert flag in the WebAssert object, defaulted to FALSE for the same behaviour as now. If the flag is FALSE then issue a deprecation warning when waitFor... is called, if the flag is TRUE then perform the assertion. Callers can opt in for a single method or an entire test class by setting the flag once in their setUp() method. In Drupal 10 remove the flag and make the methods always assert.

I think as a first step we should add new methods for this, the naming shouldn't be too hard if we follow \Drupal\FunctionalJavascriptTests\JSWebAssert::assertNoElementAfterWait

waitForElement => assertElementAfterWait
waitForElementVisible => assertElementVisibleAfterWait
waitForText => assertTextAfterWait
etc.....

Seems pretty clear to me what those would do.

Not sure we can deprecate the old methods since this would not be a 1-1 replacement.

My 2 cents - I'm for adding new assert*() methods, consistent with PHPUnit behaviour (i.e., throwing PHPUnit fail/error exceptions, using Assert primitives and : void return typehinted). Then in the future if there's opportunity to remove old ones we will deprecate etc etc. We're not new in this approach, we still have methods named along Simpletest ones to keep BC. I think in test code we can accept this.

I had an alternate idea as well. Might be a bit nuts, but hear me out.

What if, in JsWebAssert, we overrode existing methods like buttonExists() and elementExists() so that they always waited by default?

JS tests are inherently somewhat asynchronous, which is why we need these waitForStuff() methods in the first place. If we made the existing assertions wait by default, then that is simpler from a DX perspective -- no need to learn a new set of wait-friendly assertions -- and it neatly fixes the backwards compatibility problem.

So, for example, we'd have this in JsWebAssert:

/**
  * {@inheritdoc}
  */
public function buttonExists($button, TraversableElement $container = NULL, int $timeout = 10000) {
  if ($timeout) {
    $this->waitForButton($button, $timeout);
  }
  return parent::buttonExists($button, $container);
}

With this, the assertion would always wait unless $timeout is explicitly 0. I think this would benefit test writers, since they could apply the same skills used in browser tests to functional JS tests. Less opportunity to mess up.

I'm going to try implementing this in another branch of the issue fork, just to see what breaks.

#13 nice idea, although I'm not a big fan of overloading overridden methods with additional arguments, especially now that PHP is getting stricter on typing.

How about something like (pseudo)

/**
  * {@inheritdoc}
  */
public function buttonExistsAfterTimeout($button, TraversableElement $container = NULL, int $timeout = 10000) {
  $this->wait....($timeout);
  return $this->buttonExists($button, $container);
}

Re #15: that would certainly work from a technical perspective, but IMHO it's worse for DX. It requires developers to remember subtle differences between the ways that browser tests and JS tests assert stuff ("which method should I use again?"). That complexity is all but guaranteed to lead to confusion, which will in turn lead people to accidentally use the wrong assertions, which will have the cumulative effect of making the JS tests untrustworthy. It wouldn't solve the fundamental issue I'm trying to address here, which is that the DX of JsWebAssert has led to unreliable tests due to it being complicated and confusing.

I don't see how adding additional optional arguments is a problem, even with regard to PHP's typing system; we wouldn't be changing existing parameters in any way, and their existence would be completely transparent to developers most of the time. The only reason to add the additional $timeout parameter is so that developers can bypass the wait if they need to (which should, in JS tests, be a relatively rare case).

An alternative idea was to set a waitForShouldAssert flag in the WebAssert object, defaulted to FALSE for the same behaviour as now. If the flag is FALSE then issue a deprecation warning when waitFor... is called, if the flag is TRUE then perform the assertion. Callers can opt in for a single method or an entire test class by setting the flag once in their setUp() method. In Drupal 10 remove the flag and make the methods always assert.

This is similar, but slightly different, to what we did in #3091870: Fail Functional Javascript tests that throw Javascript errors. What we did there was add a failOnJavascriptConsoleErrors flag that defaults to TRUE. For this issue, maybe we would name the flag something like failOnWaitForTimedOut? Then, if the failure condition is hit (for this issue, that might be something like JSWebAssert::waitForHelper() about to return something falsey) and the flag is true, then in Drupal 9 we issue a deprecation warning, and in Drupal 10 we change that to an assertion failure.

To implement this, we need a way for JSWebAssert::waitForHelper() to communicate back to the test. One way to do this would be to make WebDriverTestBase::assertSession() pass $this to the WebDriverWebAssert constructor, and to make JSWebAssert::waitForHelper() call a public method (e.g., assertSessionWaitForTimedOut()) that we add to WebDriverTestBase. Maybe there's other ways to establish the communication without requiring the assert session to have a direct reference to the test?

WebDriverTestBase::assertSessionWaitForTimedOut() could itself check the flag and if it's true then trigger a deprecation warning. However, this would result in a deprecation warning being issued even if the test subsequently fails (e.g., due to the test itself issuing its own assertion on what it was waiting for). I don't know if that's desired or not. On the one hand, it would be issuing a deprecation extraneously, given that the test did what we want it to do, which is fail. On the other hand, if a test fails anyway, then is there any inconvenience caused by an extraneous deprecation message on top of that? Note that I asked that same question in #3091870-36: Fail Functional Javascript tests that throw Javascript errors, but no followup for that was created yet.

If we want to only issue a deprecation for when the test otherwise passes, then one way we could do that is for WebDriverTestBase::assertSessionWaitForTimedOut() to simply set a flag, and then to trigger the deprecation within WebDriverTestBase::tearDown() where we can first check to see what the test's status was.

These are just some ideas. Feel free to change any of the names or other decisions about the logic flow.

In #17 I suggested a failOnWaitForTimedOut flag based on the fact that we added a flag in #3091870: Fail Functional Javascript tests that throw Javascript errors. However, in that issue, we had a reason for the flag, as documented in its issue summary. It's possible that for this issue, we don't need a flag, if we don't think there's any legitimate reason for waitFor() to time out.

Note that if we don't need the flag, and if we don't care about adding deprecation warnings even if the test subsequently fails, then JSWebAssert could issue the deprecation warning itself without needing a reference to the test.

Just to summarize this for myself: I think that we want JS tests to basically throw deprecations in the event of "Hey, you waited for something to happen, but it never did. Even though your test passed, this will cause a failure in Drupal 10."

"Hey, you waited for something to happen, but it never did. Even though your test passed, this will cause a failure in Drupal 10."

If we say "even though your test passed", then we should actually check for that, which means passing info to the test so that it can do stuff in its tearDown(). OTOH, if we just want to issue the deprecation message regardless, it could be something like "Hey, you waited for something to happen, but it never did. This will cause the test to fail in Drupal 10." And if someone sees that message on a test that's failing anyway on D9, then they'll just think "oh ok, well my test is failing anyway, so this message isn't telling me anything useful, but it's also not lying to me."

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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