Convert web tests to JTB and KTB tests for user module part-3

Nice! Happy to see you got it running!

Since we are only dealing with the conversion of 2 tests in this patch I think we can deviate from the normal policy of keeping the changes minimal.

1. +++ b/core/modules/user/tests/src/Kernel/UserWhosOnlineBlockTest.php
   @@ -0,0 +1,117 @@
   + * Tests the 'Whos Online Block'.
   

   "Tests the Who's Online block." I'd say

2. +++ b/core/modules/user/tests/src/Kernel/UserWhosOnlineBlockTest.php
   @@ -0,0 +1,117 @@
   +class UserWhosOnlineBlockTest extends KernelTestBase {
   

   No need to prefix with User since we have the namespace to let us know we are dealing with a user test.

3. +++ b/core/modules/user/tests/src/Kernel/UserWhosOnlineBlockTest.php
   @@ -0,0 +1,117 @@
   +      'id' => strtolower($this->randomMachineName(8)),
   ...
   +      'label' => $this->randomMachineName(8),
   

   I'd would prefer a fixed string for these fields, why randomly generate one?

4. +++ b/core/modules/user/tests/src/Kernel/UserWhosOnlineBlockTest.php
   @@ -0,0 +1,117 @@
   +
   

   Extra newline not needed

5. +++ b/core/modules/user/tests/src/Kernel/UserWhosOnlineBlockTest.php
   @@ -0,0 +1,117 @@
   +      'mail' => 'user1example.com',
   ...
   +      'mail' => 'user2example.com',
   ...
   +      'mail' => 'user2example.com',
   

   Lets use valid email addresses (so with an @ somewhere), and lets make them unique

1. +++ b/core/modules/user/tests/src/Kernel/UserWhosOnlineBlockTest.php
   @@ -0,0 +1,117 @@
   +    $user1->setLastAccessTime(REQUEST_TIME);
   ...
   +    $user2->setLastAccessTime(REQUEST_TIME + 1);
   ...
   +    $inactive_time = REQUEST_TIME - (15 * 60) - 1;
   

   Request time is deprecated, lets not use it

2. +++ b/core/modules/user/tests/src/Kernel/UserWhosOnlineBlockTest.php
   @@ -0,0 +1,117 @@
   +    $content = entity_view($this->block, 'block');
   

   entity_view is deprecated, lets not use it

1. +++ b/core/modules/user/tests/src/Kernel/WhosOnlineBlockTest.php
   @@ -0,0 +1,117 @@
   +   * @var \Drupal\Core\Config\Entity\ConfigEntityStorageInterface
   ...
   +  protected $controller;
   ...
   +    $this->controller = $this->container
   ...
   +    $this->block = $this->controller->create([
   

   Given that there is just one place where you actually use it, you could remove it to be a class member and make it a local variable.

2. +++ b/core/modules/user/tests/src/Kernel/WhosOnlineBlockTest.php
   @@ -0,0 +1,117 @@
   +    $this->setRawContent($this->renderer->renderRoot($content));
   

   Did you tried using $this->render() in the kernel test base? This could make things a little bit easier.

Something like this should do the trick to get it green.

This still needs cleaning up though:
replace all the assertWaitOnAjaxRequest() with waitForElement()
why is there a js and nojs version? both use javascript since its enabled for this test, it doesn't matter how you click on the button

+++ b/core/modules/user/tests/src/FunctionalJavascript/RegistrationWithUserFieldsTest.php
@@ -0,0 +1,148 @@
+   * @var \Drupal\FunctionalJavascriptTests\JSWebAssert

Can we avoid the JSWebAssert, given it will be not the recommended way anymore in the future?

@navneet0693
Yes, it does all you need.

It is a pure implementation detail that its returning a JSWebAssert right now.

The only things I could find in this are documentation issues.

1. +++ b/core/modules/user/tests/src/FunctionalJavascript/RegistrationWithUserFieldsTest.php
   @@ -0,0 +1,148 @@
   +   * @var \Drupal\Tests\WebAssert
   ...
   +   * @var \Behat\Mink\Element\DocumentElement
   

   These need comment.

2. +++ b/core/modules/user/tests/src/FunctionalJavascript/RegistrationWithUserFieldsTest.php
   @@ -0,0 +1,148 @@
   +  private $webAssert;
   ...
   +  private $page;
   

   These should be protected

Looking good.

Thanks @navneet0693

Couple of minor questions

1. +++ b/core/modules/user/tests/src/FunctionalJavascript/RegistrationWithUserFieldsTest.php
   @@ -0,0 +1,152 @@
   +    entity_get_form_display('user', 'user', 'register')
   ...
   +    entity_get_form_display('user', 'user', 'register')
   

   Should we keep this in a local variable the first time so we can edit and save it without needing to reload?

2. +++ b/core/modules/user/tests/src/Kernel/WhosOnlineBlockTest.php
   @@ -0,0 +1,89 @@
   +
   

   nit: extra line

Adding review credits

Thanks @navneet !! This addresses #39 :)

Committed as 26aebd3 and pushed to 8.6.x.

cherry-picked as 274d50c and pushed to 8.5.x

Thanks

This broke head, reverted

So looks like the test was failing on php 5.6

Can you reproduce that locally?

If it still fails, perhaps run locally with --php=`which php56` just to be 100% sure that run-tests.sh is picking up the correct php version

Doesn't feel very stable though. Requeued again, lets get some more passes on this.

+++ b/core/modules/user/tests/src/Kernel/WhosOnlineBlockTest.php
@@ -0,0 +1,117 @@
+    $user2->setLastAccessTime(\Drupal::time()->getRequestTime() + 1);

What happens if you add a user who's last access time is far into the future? Are they considered active ? it may be that the +1 here is causing them to be in the future and if the test completes fast enough, they are not considered active, but if the test is slow enough they are.

Two things I would recommend to sort out a flapping test like this.

I would resubmit the patch, except add some changes to run-tests.sh where it *only* has this one test, and sets the repeat to something high, like 100.

then I would look at the simpletest build artifacts directory, and in it you will find the html output for that one test, as those are being saved now (we cant save them for full core runs because they're 500mb and 36000, files, but they'd show up for on e thats just on repeat)
https://dispatcher.drupalci.org/job/drupal_patches/46610/artifact/jenkin... is an example of where you would find those.

+++ b/core/modules/user/tests/src/Kernel/WhosOnlineBlockTest.php
@@ -0,0 +1,117 @@
+    $user1->setLastAccessTime(\Drupal::time()->getRequestTime());
...
+    $user2->setLastAccessTime(\Drupal::time()->getRequestTime() + 1);
...
+    $inactive_time = \Drupal::time()->getRequestTime() - (15 * 60) - 1;

let's put this into a variable and make the tolerance bigger for the inactive one.

And then follow @Mixologic's suggestion of uploading a patch that hacks run-tests.sh just to run this test several times

We still need that test ran several times, to make sure we don't introduce new random failures.

I wanted to update to SeleniumDriver which leads to a new problem that the javascript test can't test the cache tags set in the headers.

So to solve this, we can just leave the BTB version as is, without the js-nojs switch. Do the things we can do in Javascript test and then we still have the same coverage.

Other issue, when leaving a required field blank, chrome detects this and the form never submits, which means we can't check the validation messages. This is what happens locally anyway, lets see what happens on Drupal CI.

This remove the browser validation and let drupal check the required field.
I've also removed the call to the method that test the cache.

@ziomizar nice! The remaining fail is just that the javascript test now needs to extend WebDriverTestBase and not JavascriptTestBase.

Fix as suggested in #68, lets see if we get a green..

This is looking great! Just some minor clean up needed I think:

1. +++ b/core/modules/user/tests/src/FunctionalJavascript/RegistrationWithUserFieldsTest.php
   @@ -0,0 +1,146 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected $minkDefaultDriverClass = DrupalSelenium2Driver::class;
   

   This is the default in WebDriverTestBase so this can be removed

2. +++ b/core/modules/user/tests/src/FunctionalJavascript/RegistrationWithUserFieldsTest.php
   @@ -0,0 +1,146 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   

   This can just be {@inheritdoc} in the new test.

3. +++ b/core/modules/user/tests/src/FunctionalJavascript/RegistrationWithUserFieldsTest.php
   @@ -0,0 +1,146 @@
   +    // In order to check the server side validation the browser specific
   ...
   +    $session = $this->getSession();
   +    $session->executeScript("jQuery('#edit-test-user-field-0-value').prop('required', false);");
   

   Love the workaround. Doc could be a little more descriptive maybe. Maybe something like:
   In order to check the server side validation the native browser validation for required fields needs to be circumvented.

Here is improved patch from #69 based on Lendude's comment ;)

Once more :)
Here is improved patch from #69 based on Lendude's comment ;)

@ApacheEx Thank you!

All feedback has been addressed.

Just wondering how come we're not removing \Drupal\user\Tests\UserBlocksTest::testWhosOnlineBlock() since we've moved it to a kernel test?

@alexpott woops! Way to focussed on the javascript part of this! Nice catch.

Yeah we need to remove \Drupal\user\Tests\UserBlocksTest::testWhosOnlineBlock

@navneet0693 yes but we should remove it here as the change here makes \Drupal\user\Tests\UserBlocksTest::testWhosOnlineBlock() obsolete.

Nice catch, here is updated patch.

@ApacheEx thanks!

Adding review credit to @alexpott and @Mixologic for patch reviews.

Committed and pushed 9fc5c5965a to 8.7.x and a538cef929 to 8.6.x. Thanks!

diff --git a/core/modules/user/tests/src/Kernel/WhosOnlineBlockTest.php b/core/modules/user/tests/src/Kernel/WhosOnlineBlockTest.php
index 63e8d74bad..02844a1610 100644
--- a/core/modules/user/tests/src/Kernel/WhosOnlineBlockTest.php
+++ b/core/modules/user/tests/src/Kernel/WhosOnlineBlockTest.php
@@ -39,7 +39,7 @@ class WhosOnlineBlockTest extends KernelTestBase {
    */
   protected $renderer;
 
-    /**
+  /**
    * {@inheritdoc}
    */
   protected function setUp() {
@@ -49,8 +49,8 @@ protected function setUp() {
     $this->installEntitySchema('user');
 
     $this->controller = $this->container
-        ->get('entity_type.manager')
-        ->getStorage('block');
+      ->get('entity_type.manager')
+      ->getStorage('block');
 
     // Create a block with only required values.
     $this->block = $this->controller->create([

Fixed coding standards on commit.