Problem/Motivation

In preparation for #2775653: [PP-2] JavascriptTests with webDriver we need to ensure that we have no more calls to statusCodeEquals() and responseHeaderEquals().

Given that

I think we should do a hard break on statusCodeEquals and throw an exception JTB is experimental and supporting multiple versions is way more expensive. We could change drupalLogin to not use it but assert something else or just remove it since if the form fails it fails.

Proposed resolution

Add the exceptions
Remove usage in core of all response code related methods. Let the exception throwing be done by the drivers.

Remaining tasks

None.

User interface changes

API changes

No changes.

Data model changes

Comments

dawehner created an issue. See original summary.

dawehner’s picture

Issue tags: +php-novice, +Novice

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

michielnugter’s picture

Status: Active » Needs review
FileSize
94.53 KB

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.

michielnugter’s picture

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

dawehner’s picture

It would be kinda nice to be able to share most of the code with the parent class somehow.

One possibility could be to simply not do anything in the assertion.

michielnugter’s picture

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..

The last submitted patch, 4: 2827014-4.patch, failed testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Yeah agreed. We also better break earlier than later!

cilefen’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -php-novice, -Novice

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.

cilefen’s picture

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.

xjm’s picture

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.

michielnugter’s picture

@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.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
4.25 KB
1.56 KB

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.

dawehner’s picture

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
@@ -366,4 +366,32 @@ function t(r, lx, ly) {
+   * @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) {

Well, you cannot remove it in the future, as the base class contains the implementation already, right?

michielnugter’s picture

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?

dawehner’s picture

Well, how do you want to remove that. There is a class hierarchy involved.

michielnugter’s picture

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?

cilefen’s picture

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

xjm’s picture

Status: Needs review » Needs work
+++ 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.

xjm’s picture

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!

cilefen’s picture

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

xjm’s picture

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.

michielnugter’s picture

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?

cilefen’s picture

On #2775653: [PP-2] 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.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
5.03 KB
3.55 KB

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?

cilefen’s picture

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

dawehner’s picture

Great idea to remove the asssertions from drupalLogin. We test that thousand times, so we will see when this fails anyway.

- Removed deprecation warnings.

Maybe using @internal communicates a similar message?

cilefen’s picture

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

michielnugter’s picture

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?

cilefen’s picture

"@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".

michielnugter’s picture

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?

cilefen’s picture

Issue tags: +Needs change record

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

michielnugter’s picture

Removed the @internal tag and created a draft CR:

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

cilefen’s picture

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.

michielnugter’s picture

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.

cilefen’s picture

$this->statusCodeNotEquals(3);

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

michielnugter’s picture

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.

cilefen’s picture

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

dawehner’s picture

Status: Needs review » Needs work
michielnugter’s picture

Status: Needs work » Needs review
FileSize
1.71 KB
3.79 KB

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

droplet’s picture

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

cilefen’s picture

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

cilefen’s picture

michielnugter’s picture

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.

droplet’s picture

Issue tags: +JavaScript, +JavaScriptTest
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I think having some kind of documentation on the method is a good idea.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record

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.

  • alexpott committed f1b5e67 on 8.4.x
    Issue #2827014 by michielnugter, cilefen, dawehner, xjm: Throw an...
droplet’s picture

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!

  • alexpott committed 1d780fb on 8.4.x
    Revert "Issue #2827014 by michielnugter, cilefen, dawehner, xjm: Throw...
alexpott’s picture

Status: Fixed » Needs work

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.

alexpott’s picture

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

michielnugter’s picture

Status: Needs work » Needs review
FileSize
1.93 KB
3.9 KB

Right, bad one to miss, sorry!

All fixed now!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I wondered for a second whether there is any value in some sort of unit test, but I don't really believe in it.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 54: 2827014-54.patch, failed testing.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Postponed

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.

marcoscano’s picture

Status: Postponed » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 54: 2827014-54.patch, failed testing.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
2.1 KB
5.56 KB

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.

droplet’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @michielnugter.

Wim Leers’s picture

Title: Throw an exception when testing non response body in javascript tests » Throw an exception when testing status code or response headers in functional JavaScript tests
Issue summary: View changes

lgtm

Fixed IS.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 61: 2827014-61.patch, failed testing. View results

marcoscano’s picture

Status: Needs work » Needs review
FileSize
6.38 KB
1.55 KB
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Media Initiative

@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.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

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.

larowlan’s picture

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

michielnugter’s picture

Status: Needs review » Needs work

@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.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
4.02 KB

And the patch with diff.

Lendude’s picture

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

michielnugter’s picture

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.

michielnugter’s picture

Issue summary: View changes

Updated IS to reflect the changed solution.

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

dawehner’s picture

I'm a bit confused. Wasn't the point of adding these methods to inform users that things are deprecated, when they use JavascriptTestBase, and before we switch the driver?

Lendude’s picture

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?

dawehner’s picture

Well, its more than deprecated IMHO. We really don't want to support it for longer. Deprecations would last until 9.x, but as you know, we want to use a webdriver based driver before that.

michielnugter’s picture

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.

michielnugter’s picture

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?

larowlan’s picture

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?
catch’s picture

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.