Follow-up to #2870465: Convert web tests to browser tests for user module

Conversion scope

- UserAdminSettingsFormTest extending SystemConfigFormTestBase which is getting converted here #2867154: Form: Convert system functional tests to phpunit
- UserPasswordResetTest should be split into JTB and BTB
- UserBlocksTest requires setRawContent to be available in BTB
- UserRegistrationTest should be split into JTB and BTB
- UserAdminLanguageTest requires constructFieldXpath which is getting converted here #2868019: AssertLegacyTrait field assertions not compatible with Simpletest assertions
- UserCreateTest See more background details here #2870465-17: Convert web tests to browser tests for user module

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

naveenvalecha created an issue. See original summary.

naveenvalecha’s picture

Issue summary: View changes

updated IS

dawehner’s picture

These tests are independent right? In this case we don't need to postpone on it, but splitting might still make sense.

navneet0693’s picture

Status: Postponed » Active

As parent is RTBC'ed ! I am starting working on this.

navneet0693’s picture

navneet0693’s picture

Status: Active » Needs review
naveenvalecha’s picture

Issue summary: View changes

Updating IS.


@navneet0693,
Thanks for the patch, Please create the patch using git diff --diff-copies


These tests are independent right? In this case, we don't need to postpone on it, but splitting might still make sense.

Few of the tests are dependent onto to few other issues. even if we need to split them into their own issue, +1 for that. However, it will put more burden on reviewing it but it will help the contributors to push the efforts in right direction.

//Naveen

Status: Needs review » Needs work

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

navneet0693’s picture

Status: Needs work » Needs review

@naveenvalecha,

1. Thanks for pointing to issue where SystemConfigFormTestBase is getting converted, I included the same change in above stated patch itself.
2. For UserAdminLanguageTest, I will apply the changes on local and change accordingly.

Working on the other's :-)

navneet0693’s picture

Status: Needs review » Needs work

Oops my bad, changing to Needs Work!

navneet0693’s picture

Status: Needs work » Needs review
FileSize
56.13 KB

I have added few changes in patch. We may break them into JTB, but I think BTB is covering all cases currently. Please correct me if I am wrong :-)

Status: Needs review » Needs work

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

Lendude’s picture

Nice to see work on this!

Quick point:

+++ b/core/modules/user/tests/src/Functional/UserBlocksTest.php
@@ -51,10 +52,10 @@ public function testUserLoginBlockVisibility() {
+        self::assertTrue(!empty($elements), 'User login block in path "' . $path . '" should be visible');
...
+        self::assertTrue(empty($elements), 'User login block in path "' . $path . '" should not be visible');

Please keep $this->assertXXX, using self:: is not a pattern we use and it also needlessly bloats the patch, please try to keep the changes to an absolute minimum to get the tests to pass. Cleaning up deprecated code is going to be a whole different can of worms we will tackle later!

Dinesh18’s picture

Here is the updated patch and interdiff.txt which implements #13

Dinesh18’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

Lendude’s picture

Status: Needs work » Needs review
FileSize
14.35 KB
52.87 KB

@navneet0693 you asked why testUserPasswordResetLoggedIn was failing, well:

+++ b/core/modules/user/tests/src/Functional/UserPasswordResetTest.php
@@ -241,14 +252,11 @@ public function testUserPasswordResetLoggedIn() {
-    $this->assertRaw(new FormattableMarkup(
-      'Another user (%other_user) is already logged into the site on this computer, but you tried to use a one-time link for user %resetting_user. Please <a href=":logout">log out</a> and try using the link again.',
-      ['%other_user' => $this->account->getUsername(), '%resetting_user' => $another_account->getUsername(), ':logout' => Url::fromRoute('user.logout')->toString()]
-    ));
+    $web_assert->responseContains('You have tried to use a one-time login link that has either been used or is no longer valid. Please request a new one using the form below.');

This assertion is completely changed here, so that is why this test is failing. Also, passRaw was not set in the account, which is needed in BrowserTestBase.

When doing these conversions, please keep the changes as minimal as possible. If a change is not needed to make a test pass, don't make that change! These patches tend to get pretty big, so keeping them as small as possible really helps to get them reviewed.

Undid some of the unneeded changes here, but I think we can shrink this some more.

Status: Needs review » Needs work

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

navneet0693’s picture

@Lendude thanks for pointing it :-) I will keep these in mind. Working further more on #17

navneet0693’s picture

I have minimised the changes. Upload a patch for same.

navneet0693’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

navneet0693’s picture

I am not sure why this failed: /var/www/html/core/modules/user/tests/src/Functional/UserCreateTest.php:73, it was passing on local.

Attached interdiff to show replacement of constructFieldXpath().

navneet0693’s picture

Status: Needs work » Needs review
dawehner’s picture

Thank you for working on this rather hard part of the conversion!

  1. +++ b/core/modules/user/tests/src/Functional/UserAdminLanguageTest.php
    @@ -50,7 +50,7 @@ public function testUserAdminLanguageConfigurationNotAvailableWithOnlyOneLanguag
    +    $this->assertFalse($this->getSession()->getPage()->findField('edit-preferred-admin-langcode'));
    
    @@ -64,13 +64,13 @@ public function testUserAdminLanguageConfigurationAvailableWithAdminLanguageNego
    -    $this->assertNoFieldByXPath($this->constructFieldXpath('id', 'edit-preferred-admin-langcode'), NULL, 'Administration pages language selector not available.');
    +    $this->assertFalse($this->getSession()->getPage()->findField('edit-preferred-admin-langcode'));
    ...
    -    $this->assertFieldByXPath($this->constructFieldXpath('id', 'edit-preferred-admin-langcode'), NULL, 'Administration pages language selector is available.');
    +    $this->assertTrue($this->getSession()->getPage()->findField('edit-preferred-admin-langcode'));
    

    Note: findField says it returns NULL, so let's use assertNull?

  2. +++ b/core/modules/user/tests/src/Functional/UserAdminLanguageTest.php
    @@ -91,13 +91,13 @@ public function testUserAdminLanguageConfigurationAvailableIfAdminLanguageNegoti
    -    $this->assertFieldByXPath($this->constructFieldXpath('id', 'edit-preferred-admin-langcode'), NULL, 'Administration pages language selector available for admins.');
    +    $this->assertTrue($this->getSession()->getPage()->findField( 'edit-preferred-admin-langcode'));
    

    We could use assertInstanceOf here ..., not sure though this is worth doing so.

  3. +++ b/core/modules/user/tests/src/Functional/UserBlocksTest.php
    @@ -32,7 +32,7 @@ protected function setUp() {
         $this->drupalPlaceBlock('user_login_block');
    -    $this->drupalLogout($this->adminUser);
    +    $this->drupalLogout();
    

    Note: This change is not strictly needed. Maybe drop it, so noone has to look up whether WebTestBase used to support it.

  4. +++ b/core/modules/user/tests/src/Functional/UserPasswordResetTest.php
    @@ -54,12 +63,12 @@ protected function setUp() {
         $this->account = User::load($account->id());
    -    $this->account->pass_raw = $account->pass_raw;
    +    $this->account->passRaw = $account->pass_raw;
         $this->drupalLogout();
    

    I would have not expected this to work. Are you sure you don't have to change the right hand as well?

  5. +++ b/core/modules/user/tests/src/Functional/UserPasswordResetTest.php
    @@ -54,12 +63,12 @@ protected function setUp() {
    -    $account->login = REQUEST_TIME - mt_rand(10, 100000);
    +    $account->login = \Drupal::time()->getRequestTime() - mt_rand(10, 100000);
    

    Unneeded change in this conversion. Keep in mind: This is about converting to BTB, not to make the test perfect :)

  6. +++ b/core/modules/user/tests/src/Functional/UserPasswordResetTest.php
    @@ -81,11 +91,11 @@ public function testUserPasswordReset() {
    -    $this->assertText(t('@name is not recognized as a username or an email address.', ['@name' => $edit['name']]), 'Validation error message shown when trying to request password for invalid account.');
    ...
    +    $web_assert->pageTextContains(t('@name is not recognized as a username or an email address.', ['@name' => $edit['name']]));
    

    IMHO you could drop assertText->pageTextContains as well.

  7. +++ b/core/modules/user/tests/src/Functional/UserPasswordResetTest.php
    @@ -81,11 +91,11 @@ public function testUserPasswordReset() {
    -    $this->assertEqual(count($this->drupalGetMails(['id' => 'user_password_reset'])), 0, 'No email was sent when requesting a password for an invalid account.');
    ...
    +    $this->assertEquals(count($this->drupalGetMails(['id' => 'user_password_reset'])),0,'No email was sent when requesting a password for an invalid account.');
    

    As far as I see, you don't need to change from assertEqual to assertEquals here. Let's minimize the changes.

  8. +++ b/core/modules/user/tests/src/Functional/UserPasswordResetTest.php
    @@ -188,7 +198,7 @@ public function testUserPasswordReset() {
    -    $this->assertText(t('You have tried to use a one-time login link that has either been used or is no longer valid. Please request a new one using the form below.'), 'One-time link is no longer valid.');
    +    $web_assert->pageTextContains(t('You have tried to use a one-time login link that has either been used or is no longer valid. Please request a new one using the form below.'));
    
    @@ -197,21 +207,21 @@ public function testUserPasswordReset() {
         $this->drupalLogout();
    -    $timestamp = REQUEST_TIME - 1;
    +    $timestamp = \Drupal::time()->getRequestTime() - 1;
    ...
    -    $this->assertResponse(403);
    +    $web_assert->statusCodeEquals(403);
     
    ...
         $this->drupalGet("user/reset/" . $blocked_account->id() . "/$timestamp/" . user_pass_rehash($blocked_account, $timestamp) . '/login');
    -    $this->assertResponse(403);
    +    $web_assert->statusCodeEquals(403);
    
    @@ -266,41 +276,43 @@ public function testUserPasswordResetLoggedIn() {
         // user.reset.form routes.
    -    $timestamp = REQUEST_TIME - 1;
    

    There are a lot of unnecessary changes, IMHO.

  9. +++ b/core/modules/user/tests/src/Functional/UserRegistrationTest.php
    @@ -348,45 +347,37 @@ public function testRegistrationWithUserFields() {
    -      $edit['test_user_field[0][value]'] = $value;
    -      if ($js == 'js') {
    -        $this->drupalPostAjaxForm(NULL, $edit, 'test_user_field_add_more');
    -        $this->drupalPostAjaxForm(NULL, $edit, 'test_user_field_add_more');
    -      }
    -      else {
    -        $this->drupalPostForm(NULL, $edit, t('Add another item'));
    -        $this->drupalPostForm(NULL, $edit, t('Add another item'));
    -      }
    

    Do we have an issue to create the JS test?

Status: Needs review » Needs work

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

navneet0693’s picture

We could use assertInstanceOf here ..., not sure though this is worth doing so.

assertNotNull seemed more appropriate to me :-)

There are a lot of unnecessary changes, IMHO.

Changes reduced to minimal.

Do we have an issue to create the JS test?

We can split them to JTB, but assertions were kind of same and resulted the same thing.

navneet0693’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 27: 2887869-27.patch, failed testing. View results

navneet0693’s picture

Status: Needs work » Needs review
FileSize
12.79 KB

I am sorry, I missed few changes here and there. Only one failure should be there.

Status: Needs review » Needs work

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

dawehner’s picture

  1. +++ b/core/modules/user/tests/src/Functional/UserBlocksTest.php
    @@ -32,7 +32,7 @@ protected function setUp() {
         $this->drupalPlaceBlock('user_login_block');
    -    $this->drupalLogout($this->adminUser);
    

    This is still unneeded IMHO

  2. +++ b/core/modules/user/tests/src/Functional/UserPasswordResetTest.php
    @@ -120,7 +129,7 @@ public function testUserPasswordReset() {
         $edit = [
           'files[user_picture_0]' => drupal_realpath($image->uri),
         ];
    -    $this->drupalPostAjaxForm(NULL, $edit, 'user_picture_0_upload_button');
    +    $this->drupalPostForm(NULL, $edit, 'Upload');
     
         // Change the forgotten password.
    

    Can you open up a follow up to create a JS test for that?

  3. +++ b/core/modules/user/tests/src/Functional/UserRegistrationTest.php
    @@ -273,11 +272,11 @@ public function testUniqueFields() {
    -    $this->assertRaw(SafeMarkup::format('The username %value is already taken.', ['%value' => $account->getUsername()]));
    +    $this->assertRaw(t('The username %value is already taken.', ['%value' => $account->getUsername()]));
    ...
    -    $this->assertRaw(SafeMarkup::format('The email address %value is already taken.', ['%value' => $account->getEmail()]));
    +    $this->assertRaw(t('The email address %value is already taken.', ['%value' => $account->getEmail()]));
    

    Are you sure you need this change?

navneet0693’s picture

This is still unneeded IMHO

Removing this :) my concern was that in BTB drupalLogout doesn't accepts a parameter so I corrected it.

1. We will have to write a KernelTest for UserBlocksTest to test which contains setRawContent
2. And a JS test of UserPasswordResetTest

Creating a followup issue.

Are you sure you need this change?

t() does the same but we can revert that too.

navneet0693’s picture

Status: Needs work » Needs review
FileSize
4.64 KB
14.01 KB

Status: Needs review » Needs work

The last submitted patch, 34: 2887869-34.patch, failed testing. View results

navneet0693’s picture

FileSize
162.49 KB

Tests will pass once this #2868019: AssertLegacyTrait field assertions not compatible with Simpletest assertions is committed, I have applied the same on local and then tested the patch.

Attaching screenshot.

naveenvalecha’s picture

@navneet0693,
could you upload a combined patch here.

//Naveen

navneet0693’s picture

navneet0693’s picture

Status: Needs work » Needs review
navneet0693’s picture

Removed *testRegistrationWithUserFields* from BTB of *UserRegisterationTest.php* in as it moved to new JTB here: https://www.drupal.org/node/2895271#comment-12178523

dawehner’s picture

@navneet0693
I'm a bit confused. The latest patch got almost twice as big. Do you know why? It seems to be that you basically just add new code in there.

navneet0693’s picture

FileSize
33.5 KB

@dawehner I might have done some mistake while adding files in git staging.

Re-uploading patch. It contains latest patch from: #2868019: AssertLegacyTrait field assertions not compatible with Simpletest assertions

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

Status: Needs review » Needs work

Patch doesn't applies anymore. Re-rolling the patch in #34

navneet0693’s picture

navneet0693’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

navneet0693’s picture

Status: Needs work » Needs review
FileSize
13.97 KB

My bad :-( missed changing a namespace

dawehner’s picture

It feels like it would be better to fix the kernel/JTB conversion first and then continue on here?

navneet0693’s picture

@dawehner the patch is in needs review state for it: https://www.drupal.org/project/drupal/issues/2895271#comment-12189968

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 » Reviewed & tested by the community

Looks good to me. @navneet0693

Status: Reviewed & tested by the community » Needs work

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

navneet0693’s picture

Status: Needs work » Reviewed & tested by the community

Don't know what triggered test to fail and change status to needs work, setting again to RTBC.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

#49 indicates we should be waiting on something here, I assume that is #2895271: Convert web tests to JTB and KTB tests for user module part-3

Lets wait for that to land as its close

jhedstrom’s picture

Note that #2867154: Form: Convert system functional tests to phpunit did not actually convert SystemConfigFormTestBase.