Convert web tests to browser tests for user module

Here's the straight conversion. Let's see how many breaks. expecting Rest and views tests failures.

//Naveen

Removing
1) UserRegistrationTest as some part of it should be Javascript test
2) UserBlocksTest It should be postponed on some follow-up issue which converts the setRawContent b/c it's not available in BTB yet.

//Naveen

Here are the fixes of the failures.

//Naveen

Added followup issue link

Addressed #10 failures.

2) UserBlocksTest It should be postponed on some follow-up issue which converts the setRawContent b/c it's not available in BTB yet.

Actually setRawContent means it needs to be converted to a kernel test, and since it's only a little bit of test we might do it in this issue (although the patch is big enough as it is, so I'm fine doing it in the followup too)

Actually setRawContent means it needs to be converted to a kernel test, and since it's only a little bit of test we might do it in this issue (although the patch is big enough as it is, so I'm fine doing it in the followup too)

I wouldn't mind doing it here if it blocks you to RTBC it?

//Naveen

#15 I'm fine with it being converted in the follow up.

And really nice to see one of the big modules taking a big step towards conversion!

some things I see:

1. \Drupal\Tests\user\Functional\RestRegisterUserTest in extending a WebTestBase base class, it needs to extend \Drupal\Tests\rest\Functional\ResourceTestBase

2. +++ b/core/modules/user/tests/src/Functional/UserAdminLanguageTest.php
   @@ -50,7 +50,11 @@ public function testUserAdminLanguageConfigurationNotAvailableWithOnlyOneLanguag
   -    $this->assertNoFieldByXPath($this->constructFieldXpath('id', 'edit-preferred-admin-langcode'), NULL, 'Administration pages language selector not available.');
   

   constructFieldXpath is coming back in #2868019: AssertLegacyTrait field assertions not compatible with Simpletest assertions, maybe worth waiting on that. That only needs a little push to take it over the line, and it's used quite a lot here.

3. +++ b/core/modules/user/tests/src/Functional/UserCreateTest.php
   @@ -66,8 +70,15 @@ public function testUserAdd() {
   -    $this->assertFieldbyId('edit-status-0', 0, 'The user status option Blocked exists.', 'User login');
   -    $this->assertFieldbyId('edit-status-1', 1, 'The user status option Active exists.', 'User login');
   ...
   +    $status_field_1_xpath = $this->assertSession()->buildXPathQuery('//textarea[@id=:value]|//input[@id=:value]|//select[@id=:value]', [':value' => 'edit-status-0']);
   +    $status_field_1 = $this->getSession()->getPage()->find('xpath', $status_field_1_xpath);
   +    $this->assertEquals($status_field_1->getAttribute('value'), 0);
   ...
   +    $status_field_2_xpath = $this->assertSession()->buildXPathQuery('//textarea[@id=:value]|//input[@id=:value]|//select[@id=:value]', [':value' => 'edit-status-1']);
   +    $status_field_2 = $this->getSession()->getPage()->find('xpath', $status_field_2_xpath);
   +    $this->assertEquals($status_field_2->getAttribute('value'), 1);
   

   Why doesn't assertFieldById work here?

4. +++ b/core/modules/user/tests/src/Functional/UserCreateTest.php
   @@ -109,7 +120,7 @@ public function testUserAdd() {
   -      $this->assertEqual($user->isActive(), 'User is not blocked');
   +      $this->assertEqual($user->isActive(), TRUE, 'User is not blocked');
   

   haha! nice cleanup.

5. +++ b/core/modules/user/tests/src/Functional/UserRoleAdminTest.php
   @@ -46,7 +46,7 @@ public function testRoleAdministration() {
   -      ':text' => t('Roles'),
   +      ':text' => 'Roles',
   

   Is this needed to make this pass? Otherwise lets keep this as one-on-one as we can

#16.1 Filed a follow-up for this to discuss this in rest queue #2889399: Remove RestRegisterUserTest? Waiting for it if not will convert if it's really required.
#16.2 we could postpone it on that issue. We're blocking the whole conversion on a single item.How about removing this test out of the scope?
#16.3
AssertLegacyTrait::assertFieldById compares the value of the checkbox using getvalue() which would always be false either its checked or not checked. $this->assertEquals($value, $field->getValue());

  protected function assertFieldById($id, $value = '') {
    $xpath = $this->assertSession()->buildXPathQuery('//textarea[@id=:value]|//input[@id=:value]|//select[@id=:value]', [':value' => $id]);
    $field = $this->getSession()->getPage()->find('xpath', $xpath);

    if (empty($field)) {
      throw new ElementNotFoundException($this->getSession()->getDriver(), 'form field', 'id', $field);
    }

    if ($value !== NULL) {
      $this->assertEquals($value, $field->getValue());
    }
  }

As per the documentation of \Behat\Mink\Element::getValue(), For checkbox fields, the value is a boolean indicating whether the checkbox is checked but it didn't work. See the code below for reference.

    /**
     * Returns the value of the form field or option element.
     *
     * For checkbox fields, the value is a boolean indicating whether the checkbox is checked.
     * For radio buttons, the value is the value of the selected button in the radio group
     *      or null if no button is selected.
     * For single select boxes, the value is the value of the selected option.
     * For multiple select boxes, the value is an array of selected option values.
     * for file inputs, the return value is undefined given that browsers don't allow accessing
     *      the value of file inputs for security reasons. Some drivers may allow accessing the
     *      path of the file set in the field, but this is not required if it cannot be implemented.
     * For textarea elements and all textual fields, the value is the content of the field.
     * Form option elements, the value is the value of the option (the value attribute or the text
     *      content if the attribute is not set).
     *
     * Calling this method on other elements than form fields or option elements is not allowed.
     *
     * @return string|bool|array
     */
    public function getValue()
    {
        return $this->getDriver()->getValue($this->getXpath());
    }

#16.4 :)
#16.5 yup it was required to make the tests pass.

P.S: I have not updated the issue status, please change it accordingly.

//Naveen

#16.1
We required this test coverage as per the discussion in the #2889399: Remove RestRegisterUserTest? Created a followup issue so that we could convert all RestTestbase tests in the single go. #2889882: Convert ResponseGeneratorTest, DbLogResourceTest, RestRegisterUserTest to BTB
Updated IS to also remove RestRegisterUserTest from this patch in favor of #2889882: Convert ResponseGeneratorTest, DbLogResourceTest, RestRegisterUserTest to BTB
Back to N/R
//Naveen

Removed the UserAdminLanguageTest and UserCreateTest as they require the #2868019: AssertLegacyTrait field assertions not compatible with Simpletest assertions and doing it in follow-up as per word with @lendude over IRC. Let's convert the tests here which has minimum disruptions.
Also updated IS of #2887869: Convert web tests to browser tests for user module part-2
//Naveen

Looks great. We have a follow up for the remaining tests.

1. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
   @@ -10,7 +11,9 @@
   +  use AssertMailTrait;
   
   @@ -180,7 +185,7 @@ public function testNotificationEmailAddress() {
   -    $admin_mail = $this->drupalGetMails([
   +    $admin_mail = $this->getMails([
   
   +++ b/core/modules/user/tests/src/Functional/UserPictureTest.php
   @@ -1,17 +1,22 @@
   +  use TestFileCreationTrait {
   +    getTestFiles as drupalGetTestFiles;
   +  }
    
   

   We don't use the mail method as drupalGetMails() but we do so for drupalGetTestFiles() we do. Why?

2. +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
   @@ -132,7 +132,7 @@ public function testPasswordRehashOnLogin() {
   -    $account->pass_raw = $password;
   +    $account->passRaw = $password;
   
   @@ -145,7 +145,7 @@ public function testPasswordRehashOnLogin() {
   -   *   A user object with name and pass_raw attributes for the login attempt.
   +   *   A user object with name and passRaw attributes for the login attempt.
   
   @@ -157,7 +157,7 @@ public function testPasswordRehashOnLogin() {
   -      'pass' => $account->pass_raw,
   +      'pass' => $account->passRaw,
   

   Why are these changes needed to convert the test? (And there are a lot more of them).

21.1 Yeah makes sense to al least make the them use the same pattern, don't really care which one, with or without the drupal prefix
21.2 \Drupal\Tests\BrowserTestBase::drupalLogin vs \Drupal\simpletest\WebTestBase::drupalLogin, BrowserTestBase uses passRaw, I assume so that the new testbase follows Drupal naming conventions for class properties (which pass_raw doesn't do)

Addressed comments in #21/#22:

1. Use the same pattern in UserAdminTest as in UserPictureTest.
2. No corrections made - retaining changes that follow Drupal naming conventions.

Bleh, comment got eaten.

Needed a reroll, and improved the failing test, didn't look into why that started failing now.

How did this come back green before?

Oh well, fixed fails.

@vaplas nice clean up.

I think this is almost ready, but lets not do #32.4, that seems way out of scope since we aren't even converting that test.

bump

All feedback has been addressed, we have a followup for the remaining tests, back to RTBC

Committed 6648bb6 and pushed to 8.5.x. Thanks!

a) I don't we need to commit it to 8.4.x
b) Catch pushed it nows, so I think we are fine

@vaplas
For me its like: By default don't backport it, unless there is a good reason. Bugfixes for example might have a good reason.

@dawehner I think it would be nice if this did get a backport. All the other conversions have been backported and its nice to keep the testsuite in sync. This minimises the chances of having to rewrite tests for bugfixes to apply to 8.5.x and 8.4.x
Strictly speaking this is a test-only change so its eligible for backport, but it could be called disruptive so it might be ineligible because of that.... *shrug*

Yeah there is the advantage that backporting bugfixes for the user module becomes potentially easier with this.