Throw an exception when testing status code or response headers in functional JavaScript tests

Just to get the ball rolling here.

- Removed the status check from SessionTest. It will fail anyway on the next click if it's an error page.
- Reimplemented login/logout and conform them to the JTB way of testing.
- statusCodeEquals() and statusCodeNotEquals() now throw exceptions. Not sure yet on the doxygen and the type of Exception.

Sorry, garbage of a previous patch still in this one.. New patch.

That's true but that would make it impossible to do a hard break on these methods as @alexpott suggested. It would also mean we have methods that imply a test that is not actually performed.

I don't see any way around this..

Thank you both for working on this. I would rather have an exception than think a test passed when it did not! This issue is not specified enough for a novice to begin with, so I removed those tags.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
@@ -366,4 +366,32 @@ function t(r, lx, ly) {
+   * @deprecated in Drupal 8.4.x, will be immediately removed Drupal 8.8.x.
+   *             Directly check the DOM to assert the current status is what's
+   *             expected.
...
+   * @deprecated in Drupal 8.4.x, will be immediately removed Drupal 8.8.x.
+   *             Directly check the DOM to assert the current status is what's
+   *             expected.

I have two nitpicks: Lines after docblock tags are indented two spaces and docblock summaries must be less than 80 characters.

I am not sure about the "immediately" part of the deprecation—we do not use that term in any other deprecation comments. Also, we usually write "removed before". Why is the deprecation targeted to "8.8.x"? Is this is part of the test system roadmap? The issue summary does not explain and there is no change record.

In terms of developer experience, "check the DOM" isn't the most helpful tip as a replacement. The patch does not follow its own advice of checking the DOM and simply removes a call in testSessionExpiration, which I find odd.

Let's get a change record drafted.

Removed the status check from SessionTest. It will fail anyway on the next click if it's an error page.

If that's true, ignore that part of the review above.

Thanks @cilefen. The deprecation message should follow our normal pattern. We don't remove APIs in minors, so it should say "before 9.0.0" like other things.

@cilefen:
8.8.x was a typo, my bad! I meant 8.4.x. The alternative to this will be different for each situation but I'll try to write a more helpful comment.

@xjm:
Because an exception is thrown it's actually an immediate bc-break as tests would fail if they still use these methods. It's why I added a comment on the immediate deprecation instead of the normal deprecation comment.

If we were to switch drivers (#2775653, it's why this issue was opened) a break is needed as Webdriver doesn't support fetching the response code.

Fixed the versions in the deprecated message and conformed more to the standards. I tried to add a better suggestion for the alternative.

Would like feedback on the versions, this is my interpretation for what it should be based on the fact 8.3 is in alpha already.

Just to be sure I understand correctly: Do you mean we cannot remove this untill 9.x? If that's the case we will have to wait with the driver change for JTB tests if we want these methods to keep working as they currently do right?

Wow, ok, not a proud moment for me. To be clear, I actually do know about object oriented programming ;)

So ok, maybe we can commit a deprecation warning on 8.4.x and call the parent method (keep it working). In 8.5.x we can then add the exception and break that way.

Is that workable? Should I open a separate issue for the deprecation message and upgrade the version on this issue to 8.5.x?

Other question: can we still add the deprecation to 8.3.x or is it too late for that?

As for the 8.3 question, here are the rules: beta phase is now, https://www.drupal.org/core/d8-allowed-changes#beta, RC begins soon https://www.drupal.org/core/d8-allowed-changes#rc

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
@@ -366,4 +366,32 @@ function t(r, lx, ly) {
+  /**
+   * The use of statusCodeEquals() is not available in a functional JavaScript test.
+   *
+   * @throws \Exception
+   *   Throws an exception on use.
+   *
+   * @deprecated in Drupal 8.4.x, will be removed before Drupal 8.5.x. Use an
+   *             assertion on the DOM to validate what is expected. For example
+   *             assert that an element is available on the page.
+   */
+  public function statusCodeEquals($code) {
+    throw new \Exception('The use of statusCodeEquals() is not available in a functional JavaScript test.');
+  }
+
+  /**
+   * The use of statusCodeNotEquals() is not available in a functional JavaScript test.
+   *
+   * @throws \Exception
+   *   Throws an exception on use.
+   *
+   * @deprecated in Drupal 8.4.x, will be removed before Drupal 8.5.x. Use an
+   *             assertion on the DOM to validate what is expected. For example
+   *             assert that an element is not available on the page.
+   */
+  public function statusCodeNotEquals($code) {
+    throw new \Exception('The use of statusCodeNotEquals() is not available in a functional JavaScript test.');
+  }
+

Coding standards are all off here. Also, I think a specific typed exception would be better than \Exception.

Also, I don't see the point of adding methods for one minor release; that's almost more disruptive. So I would not take that approach. Thanks for working on this!

I do not understand what can be deprecated. An exception should be fine. Right?

Also, while the test API is a public API, it's also critically important for it to be easily debugged and fail explicitly. So we have a bit more leeway for new hard breaks/fails/new exceptions than we would for production APIs. And it's definitely allowed to make make failures more explicit in a minor release. Passing tests that should not pass are bad.

We need to add the methods because we don't want to disable them for normal BrowerTestBase tests, only for JavascriptTestBase tests.

I implemented a deprecated warning because contribs might still be using these methods. I figured giving the contribs 1 minor to update their tests before throwing exceptions is the correct way to do it.

But maybe I'm thinking too complicated and we should just do als @cilefen suggested add the exception and don't mention anything about deprecation?

On #2775653: JavascriptTests with webDriver, klausi recommended an UnsupportedDriverActionException. It seems klausi, dawehner, and alexpott reached consensus on throwing an exception so just do that. As for the reimplementation of drupalLogin, alexpott wrote:

We could change drupalLogin to not use it but assert something else or just remove it since if the form fails it fails.

By "remove it", alexpott is referring to removing the call to statusCodeEquals. Therefore that would be easier than reimplementing drupalLogin/drupalLogout.

I just noticed the issue summary refers to responseHeaderEquals but that is not in the patch.

Ok, now it see it, thanks for clearing it up and sorry it took me this long to get it.

I updated the patch with the following changes:
- Removed drupalLogin/drupalLogout from JavascriptTestBase
- Removed the use of the statusCode equals calls from WebTestBase
- Changed Exceptions to UnsupportedDriverActionException, makes a lot of sense.
- Throws exceptions on responseHeaderEquals and Not variant.
- Removed deprecation warnings.

On the last one I'm not 100% sure. It's nice to have your IDE point out that the use of a method is not recommeded, the @deprecated tag adds a strike-through, very handy feature. Is there any other way for this?

There is #2856744: [META] Add trigger_error(..., E_USER_DEPRECATED) to deprecated code. But "deprecated" implies "will be removed". That is not the case here.

Semantically, @internal makes it seem as if the functions are useful to core itself. Hmm...

I added an @interal tag and does add the strike-through but to make phpStorm understand it, the phpdoc for assertSession() should be updated and define the actual @return type instead of {@inheritdoc}.

I think we should add the @internal. Maybe open a new issue to fix the phpdoc for assertSession?

"@internal" means "you had better know what you are doing calling this, because we may change it any time". That does not seem to apply here, which is "do not call this".

Very true, that leaves us with not adding anything. I think it's acceptable because the exception will be thrown in a test and it will be caught while in development.

Do we have to add a hint for an alternative in the phpdoc or is it ok as it is?

You can explain that in a change record. Start a draft.

Removed the @internal tag and created a draft CR:

https://www.drupal.org/node/2857562

Good! There is a field on the CR to affiliate it back to this issue, which you should do. Add something like "these will now throw exceptions if called from..." and provide example before and after usage.

I tried the link the first time but screwed the input up, fixed it.

I updated the text to mention the exception and added an example.

$this->statusCodeNotEquals(3);

Looks like some debug code is there. Also be sure to scan the files with phpcs and Drupal's coding standard.

Removed code and updated the thrown exceptions. I have phpcs enabled (again). I fixed the line lengths for the phpdoc. I didn't fix the documentation for the params, these weren't available on the original methods in Mink and it seems like wasted effort to document them now.

responseHeaderNotEquals has a wrong exception message. The param docs must meet coding standards or we can't commit.

Fixed the exception message and added param descriptions, was that what was wrong with the phpdoc?

I don't know correct or not in Drupal, which may only need to drop {@inheritdoc} for docs.

I must check that but I think the docs generator supports @inheritdoc only if it is alone.

Yes, you cannot extend a parent method's comment with {@inheritdoc} in Drupal.

But when we use {@inheritdoc} we cannot indicate in the phpdoc that something is going on in this method. I'm in favor of overwriting the phpdoc as it does right now.

Committed f1b5e67 and pushed to 8.4.x. Thanks!

I've credited @cilefen, @dawehner and @xjm for all making review comments that influenced the direction of the patch. @droplet thank you for your reviews. @michielnugter thank you for the patch.

Great!

It should credit me, haha. the basic patch come from mine: https://www.drupal.org/node/2775653#comment-11646973

** Just a joke, don't modify the GIT to create extra noise :p

Thanks ALL!

I've reverted this change because it is broken. Revealed by https://www.drupal.org/pift-ci-job/663558

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
@@ -366,4 +366,60 @@ function t(r, lx, ly) {
+    throw new UnsupportedDriverActionException('The use of statusCodeEquals() is not available in a functional JavaScript test.');

The constructor for this exception is:

    /**
     * Initializes exception.
     *
     * @param string          $template what is unsupported?
     * @param DriverInterface $driver   driver instance
     * @param \Exception      $previous previous exception
     */
    public function __construct($template, DriverInterface $driver, \Exception $previous = null)

These all should have $this passed in.

So @droplet you'll be credited when this gets in :)

Right, bad one to miss, sorry!

All fixed now!

Postponing this on #2831274: Bring Media entity module to core as Media module since that patch is very nearly and will be simple to fix in this issue once that is in.

Back to RTBC, once #2831274: Bring Media entity module to core as Media module is in.

It needed to drop the status checks in the media module.

I checked each status check and they were all followed with assertions that only pass if the status code was as expected so no actual coverage is lost.

Thanks @michielnugter.

lgtm

Fixed IS.

This probably makes #2878112: Fix Media JavaScript tests to account for changes in JavascriptTestBase obsolete.

@marcoscano's changes look legit to me. Back to RTBC we go.

Tagging this as part of the Media initiative since it touches a Media test.

Sorry for being daft, but can someone explain why we need to embed knowledge of the driver into our web assert handler.

Mink is meant to be driver agnostic.

The driver is responsible for telling us what it does and doesn't support.

For example we don't have knowledge in BTB that you can't drag and drop or take screenshots.

If you were to call this in BTB

$this->getSession()->getScreenshot()

The CoreDriver would throw the Exception because by default it is not supported - each driver then implements as appropriate.

The same should apply here.

CoreDriver throws this exception on our behalf for statusCodeEquals (the code of which should end up calling \Behat\Mink\Driver\DriverInterface::getStatusCode).

So I understand why we're removing the asserts, because depending on the driver, they may fail.

But I don't understand why we need to embed knowledge of a driver's capabilities into JsWebAssert.

We can avoid the API break altogether if we just defer this to the driver (as per the Mink design).

Am I missing something obvious or are we using Mink wrong in so far as the driver doesn't get a chance?

In summary: I think we only need to remove the assert calls if we want *core* to support both - but I don't think we should be including knowledge of a particular driver in the JsWebAssert class.

tl;dr I suspect that anyone using those methods in contrib will get this exception already if they are using that method and they switch drivers - i.e. we're just duplicating something that Mink already handles

@larowlan

You're totally right. If you'd switch driver you will get the error anyway so implementing it for ourselves is incorrect. Setting to needs work, can't reroll the patch right now without the status methods, might have time later today.

And the patch with diff.

should we do something with \Drupal\FunctionalTests\AssertLegacyTrait::assertResponse? Maybe update the docblock to indicate that this probably won't work?

Good catch. I searched the codebase and it's currently not used in FunctionalJavascript tests.

With the same logic as for the statusCode methods I think it's safe and good to let it be as is. Don't build expected driver logic into the general codebase.

Updated IS to reflect the changed solution.

Do we still need a change record if we don't break the API?

inform users that things are deprecated, when they use JavascriptTestBase

hmm so not throw an exception but just implement the methods to add @deprecated to them and then call the parent, and still leave it to the driver to throw the exception. Something like that?

I pinged @larowlan on IRC to ask if he could give his opinion on this now that it's clear again why we did the exceptions.

My 2 cents:
+1 on not deprecating then as we want to get rid of it earlier than 9.x as we have to switch off phantomJS some time.

Exception mean a hard, clear and immediate break. As we can't keep supporting them I'm in favor of this. As the driver will eventually take care of the exception I am in favor of doing a follow-up on the webdriver issue to remove the methods with exceptions again. Is that a workable solution?

log of irc conversation from this morning

05:29 <drupalbot> larowlan: 9 hours 11 min ago <michielnugter> tell larowlan Can you maybe give your opinion on https://www.drupal.org/node/2827014 now that a reason for the exceptions has been given?
05:29 <larowlan> dawehner: ping
05:29 <larowlan> lendude: ping
05:29 <lendude> larowlan: pong
05:29 <larowlan> lendude: hey thought might be quicker to talk about that issue ^
05:30 <larowlan> my concern is, if we add the exceptions we're moving from driver agnostic to webdriver
05:30 <larowlan> there are plenty of client projects out there that would have ci tied up to phantomjs
05:30 <larowlan> just because core switches to webdriver, doesn't mean everyone else is
05:30 <larowlan> suddenly we make JsWebAssert only work with webdriver, when there is no real need to
05:31 <larowlan> none of those methods are from our code, all are from Mink
05:31 <lendude> larowlan: yeah I understand your concerns and agree, but it would be nice to give people some indication that we are dropping support before we start throwing exceptions right?
05:31 <larowlan> how does the patch do that? it just starts throwing exceptions anyway
05:31 <larowlan> with the earlier rtbc patch, it starts throwing exceptions and is rigid
05:32 <larowlan> with my proposal, it throws exceptions and is flexible
05:32 <lendude> larowlan: yeah I was actually hoping for a patch that just adds @deprecated or something along those lines
05:32 <larowlan> right, not sure deprecated is right either
05:32 <larowlan> the api is in mink
05:32 <dawehner> larowlan: pong
05:32 <larowlan> dawehner: hey, did you get scroll back?
05:32 <dawehner> longwave: i dropped of for a minutre
05:33 <lendude> larowlan: yeah not sure it’s right either, but we can’t tag something as ‘feel fre to use but we don’t support it in core’
05:33 <larowlan> lendude: but we do support it in core
05:33 <larowlan> lendude: it is drupal-ci that won't support it
05:34 <larowlan> unless i'm missing something 
05:34 <lendude> larowlan: well your tests will fail if you run them on d.o, so can’t really use it in contrib, so ‘support’ would be a bit strong I think
05:34 <dawehner> well JavascriptTestBase won't support it one day, as we use a different driver by default
05:34 <larowlan> right, so then do we need a new base class
05:35 <dawehner> I guess we have to, if we treat testing with those strict API BC rules
05:35 <larowlan> because there are a lot of client projects out there with tests written on JTB and ci built around phantom
05:35 <larowlan> other wise, I think change record is enough
05:35 <larowlan> 'coming in 8.4 JTB will use webdriver by default which means these methods no longer work x, y, z'
05:36 <larowlan> 'if you want to continue to use phantomjs, you need to subclass JTB and switch drivers, then make your tests extend from that'
05:36 <larowlan> 'note drupal-ci will no longer support phantomjs'
05:36 <larowlan> and then let mink api handle the rest
05:39 <lendude> larowlan: I love the idea of letting mink handle the rest, just not thrilled about the idea of going from full support to throwing exceptions with just a CR
05:39 <lendude> larowlan: and I’m not usually that BC-minded :)
05:39 <larowlan> lendude: but that's what the last patch did too
05:39 <lendude> larowlan: yeah, with more time to think about it, not a fan of that either :)
05:40 <larowlan> i will discuss with xj_m and catc_h - maybe we do a g.d.o post now warning of the coming change
05:40 <larowlan> and then a change record when it goes in
05:40 <larowlan> and a release notes entry for 8.4
05:42 <lendude> larowlan: yeah, it might be complete overkill, just difficult to asses how much depends on this outside core
05:42 <larowlan> ok, i'll chat with them and get back to you
05:42 <lendude> larowlan++
05:43 <lendude> larowlan: just to be clear, I’m totally in favor of your solution over the original patch
05:47 <larowlan> lendude: thanks
05:48 <larowlan> dawehner: lendude out of interest, is the drupal-ci work to support this ready to go?
05:48 <dawehner> I don't think it is :(
05:49 <larowlan> thanks
05:49 <larowlan> so thats another reason to not go with the exception
05:49 <dawehner> larowlan: but that's the thing. In previous discussions we said we need the exception/errror/notice now, so we can actually switch over
05:49 <dawehner> would @trigger_error() work better here?
05:50 <larowlan> what would the message say 'Support for this method is to be dropped shortly'?
05:51 <larowlan> dawehner: how about this instead, we subclass JsWebAssert with WebdriverWebAssert and put the exceptions/trigger_error there
05:52 <dawehner> larowlan: and then use it from within JTB?
05:52 <larowlan> dawehner: then when we switch drivers, if someone wants to keep using JsWebAssert with phantom, their JTB sub-base-class overrides ->assertSession() and uses the old JsWebAssert
05:53 <dawehner> larowlan: ah clever
05:53 <larowlan> dawehner: we could even ship that class in core, as deprecated 'LegacyJsTestBase' which has the old phantom driver and the old web assert
05:54 <dawehner> larowlan: Then we could explain that in the change record and people can at least switch over easily
05:54 <larowlan> and convey in the change record that if you want to keep using phantom on your own ci infra, switch you classes to extend from that
05:54 <larowlan> ..what you said
05:54 <larowlan> lendude: that work for you?
05:54 <dawehner> larowlan: its tricky, we would ideally add it to current and next minor at the same time, so people can keep their contrib tests working
05:54 <larowlan> dawehner: yeap
05:55 <lendude> larowlan: absolutely, that sounds really nice
05:55 <larowlan> dawehner: also, in this approach, only WebDriverWebAssert is locked to webdriver, but that is fine
05:55 <larowlan> lendude: dawehner I'll post this irc log on issue if that's ok?

I'm not sure I can see exactly what #80 looks like in terms of inheritance, but the concept seems fine and better than just throwing the exception.

So this is my interpretation of our IRC discussion.

This looks great - thanks @Lendude

Updating issue credits, adding myself for reviews and Wim Leers for issue summary updates.

Committed as 6496b03 and pushed to 8.5.x.

Cherry-picked as b1657f0 and pushed to 8.4.x.

Published the change record.