Follow-up to #2887869: Convert web tests to browser tests for user module part-2

Scope of conversion:

- UserBlocksTest's testWhosOnlineBlock to KTB
- UserRegistrationTest's JS part (drupalPostAjaxForm) in testRegistrationWithUserFields to JTB.

Once the patch for JTB is in, remove testRegistrationWithUserFields from https://www.drupal.org/node/2887869#comment-12172409.

CommentFileSizeAuthor
#62 2895271-62.patch9.88 KBnavneet0693
#62 interdiff-58-62.txt1.49 KBnavneet0693
#58 interdiff-41-58.txt2.98 KBnavneet0693
#58 2895271-58.patch9.89 KBnavneet0693
#57 Issues #2895271 - php56 test - unstable.png350.24 KBnavneet0693
#53 Issues #2895271 - php56 test.png229.09 KBnavneet0693
#41 interdiff-37-41.txt1.96 KBnavneet0693
#41 2895271-41.patch9.31 KBnavneet0693
#37 2895271-39.patch9.31 KBnavneet0693
#37 interdiff--2895271-30-37.txt786 bytesnavneet0693
#30 2895271-30.patch9.24 KBnavneet0693
#30 interdiff-25-30.txt617 bytesnavneet0693
#25 2895271-25.patch9.26 KBnavneet0693
#25 interdiff-24-25.txt4.08 KBnavneet0693
#24 2895271-24.patch9.85 KBLendude
#24 interdiff-2895271-20-24.txt2.2 KBLendude
#20 2895271-20.patch9.66 KBnavneet0693
#17 2895271-17.patch9.61 KBnavneet0693
#17 interdiff-14-17.txt3.99 KBnavneet0693
#14 2895271-14.patch9.57 KBnavneet0693
#14 interdiff-12-14.txt2.92 KBnavneet0693
#12 2895271-12.patch9.62 KBnavneet0693
#12 interdiff-7-12.txt6.61 KBnavneet0693
#11 2895271-11.patch10.36 KBnavneet0693
#11 interdiff-7-11.txt7.34 KBnavneet0693
#9 2895271-test_Results.png154.55 KBnavneet0693
#7 2895271-7.patch3.08 KBnavneet0693
#7 interdiff-4-7.txt3.12 KBnavneet0693
#4 2895271-4.patch3.63 KBnavneet0693
#4 interdiff-2-4.txt2.89 KBnavneet0693
#2 2895271-1.patch3.54 KBnavneet0693
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

navneet0693 created an issue. See original summary.

navneet0693’s picture

Status: Active » Needs review
FileSize
3.54 KB

This patch only contains UserWhosOnlineBlock kernel test.

Lendude’s picture

Status: Needs review » Needs work

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

navneet0693’s picture

Status: Needs work » Needs review
FileSize
2.89 KB
3.63 KB
dawehner’s picture

  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.

Status: Needs review » Needs work

The last submitted patch, 4: 2895271-4.patch, failed testing. View results

navneet0693’s picture

Status: Needs work » Needs review
FileSize
3.12 KB
3.08 KB

Thanks @dawehner. I did the changes.

One question for converting drupalPostAjaxForm to JTB for UserRegistrationTest:: testRegistrationWithUserFields().

Refer: http://cgit.drupalcode.org/drupal/tree/core/modules/user/src/Tests/UserR...

Do I need to write a same test function again with relative changes just for testing js part? If not, please guide me with the steps.

Status: Needs review » Needs work

The last submitted patch, 7: 2895271-7.patch, failed testing. View results

navneet0693’s picture

FileSize
154.55 KB

Ouch! that is not suppose to happen, but somehow it did :-( attaching a screenshot of local test results.

navneet0693’s picture

Status: Needs work » Needs review

Added the test again, and it passed ! Hurray, now working on adding JTB.

navneet0693’s picture

Status: Needs review » Needs work
FileSize
7.34 KB
10.36 KB

Adding JTB, it needs reviews. RegistrationWithUserFieldsTest.php is failing at Line 114.

Added interdiff which only contains RegistrationWithUserFieldsTest.php for easy review.

navneet0693’s picture

Status: Needs work » Needs review
FileSize
6.61 KB
9.62 KB

Please ignore patch in #11. Failure reasons are same though.

Status: Needs review » Needs work

The last submitted patch, 12: 2895271-12.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

navneet0693’s picture

navneet0693’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: 2895271-14.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

navneet0693’s picture

FileSize
3.99 KB
9.61 KB
navneet0693’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 17: 2895271-17.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

navneet0693’s picture

I am kind of stuck here:

$field_test_user_add_more = $this->page->find('css', 'input[name="test_user_field_add_more"]');
$field_test_user_add_more->click();
$this->webAssert->assertWaitOnAjaxRequest();
$field_test_user_add_more->click();
$this->webAssert->assertWaitOnAjaxRequest();

Just after first click, I can see that *please wait* is appearing content output, which says that button is getting clicked. But it's not getting added.

navneet0693’s picture

Status: Needs work » Needs review
navneet0693’s picture

Issue summary: View changes
Issue tags: +phpunit initiative

Status: Needs review » Needs work

The last submitted patch, 20: 2895271-20.patch, failed testing. View results

Lendude’s picture

Status: Needs work » Needs review
FileSize
2.2 KB
9.85 KB

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

navneet0693’s picture

dawehner’s picture

+++ 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’s picture

Do we want to simply rely on WebAssert ?

dawehner’s picture

@navneet0693
Yes, it does all you need.

dawehner’s picture

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

navneet0693’s picture

Status: Needs review » Needs work

The last submitted patch, 30: 2895271-30.patch, failed testing. View results

navneet0693’s picture

Status: Needs work » Needs review

Running tests again!

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

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

navneet0693’s picture

Adding test against 8.5.x.

Version: 8.5.x-dev » 8.6.x-dev

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

pasan.gamage’s picture

Status: Needs review » Needs work

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

navneet0693’s picture

Status: Needs work » Needs review
FileSize
786 bytes
9.31 KB
pasan.gamage’s picture

Status: Needs review » Reviewed & tested by the community

Looking good.

Thanks @navneet0693

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

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

larowlan’s picture

Adding review credits

navneet0693’s picture

JayKandari’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @navneet !! This addresses #39 :)

  • larowlan committed 26aebd3 on 8.6.x
    Issue #2895271 by navneet0693, Lendude, dawehner, pasan.gamage, larowlan...

  • larowlan committed 274d50c on 8.5.x
    Issue #2895271 by navneet0693, Lendude, dawehner, pasan.gamage, larowlan...
larowlan’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed as 26aebd3 and pushed to 8.6.x.

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

Thanks

  • larowlan committed 237d23a on 8.5.x
    Revert "Issue #2895271 by navneet0693, Lendude, dawehner, pasan.gamage,...
larowlan’s picture

Version: 8.5.x-dev » 8.6.x-dev
Status: Fixed » Needs work

This broke head, reverted

  • larowlan committed 71550b1 on 8.6.x
    Revert "Issue #2895271 by navneet0693, Lendude, dawehner, pasan.gamage,...
navneet0693’s picture

navneet0693’s picture

navneet0693’s picture

Status: Needs work » Needs review
larowlan’s picture

Status: Needs review » Needs work

So looks like the test was failing on php 5.6

Can you reproduce that locally?

navneet0693’s picture

@larowlan Nope, the tests passed with php5.6 also, attaching screenshot for the same.

screenshot of php56 test

larowlan’s picture

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

navneet0693’s picture

Status: Needs work » Needs review

And all tests are green now !

Lendude’s picture

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

navneet0693’s picture

Status: Needs review » Needs work
FileSize
350.24 KB

Setting it to 'Needs Work' . @Lendude @larowlan This is unstable, I can reproduce on local. The test passes on first attempt, but fails in second attempt. And this is same result we can see on drupalci test runner.

Attaching screenshot:

php56 test unstable

navneet0693’s picture

Mixologic’s picture

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

larowlan’s picture

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

navneet0693’s picture

@Mixologic thanks ! I will try that. @larowlan Sure, but I wonder why there's no such random failure with http://cgit.drupalcode.org/drupal/tree/core/modules/user/src/Tests/UserB... which same logic.