Problem/Motivation

In potential preparation of #2775653: [PP-2] JavascriptTests with webDriver we need to ensure that calling 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

Remaining tasks

Write a quick change record for it.

User interface changes

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