See #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)

Conversion scope

All except: #2887869: Convert web tests to browser tests for user module part-2
- UserAdminSettingsFormTest
- UserPasswordResetTest
- UserBlocksTest
- UserRegistrationTest
- UserAdminLanguageTest
- UserCreateTest
- RestRegisterUserTest #2889882: Convert ResponseGeneratorTest, DbLogResourceTest, RestRegisterUserTest to BTB

CommentFileSizeAuthor
#41 interdiff-36-40.txt1.42 KBvaplas
#39 d84-2870465-36.patch31.2 KBvaplas
#36 interdiff-34-36.txt699 bytesvaplas
#36 2870465-36.patch32.31 KBvaplas
#34 interdiff-32-34.txt1.51 KBvaplas
#34 2870465-34.patch32.35 KBvaplas
#32 interdiff-31-32.txt5.37 KBvaplas
#32 2870465-32.patch33.87 KBvaplas
#31 2870465-31.patch32.38 KBLendude
#31 interdiff-2870465-28-31.txt1.33 KBLendude
#28 2870465-28.patch31.27 KBLendude
#28 interdiff-2870465-24-28.txt1.07 KBLendude
#24 interdiff-19-24.txt1.39 KBJo Fitzgerald
#24 2870465-24.patch30.57 KBJo Fitzgerald
#19 interdiff-2870465-18-19.txt6.94 KBnaveenvalecha
#19 2870465-19.patch31.62 KBnaveenvalecha
#18 interdiff-2870465-12-17.txt622 bytesnaveenvalecha
#18 2870465-17.patch38.56 KBnaveenvalecha
#13 interdiff-2870465-10-12.txt1.08 KBnaveenvalecha
#13 2870465-12.patch39.17 KBnaveenvalecha
#10 interdiff-2870465-8-10.txt4.9 KBnaveenvalecha
#10 2870465-10.patch38.65 KBnaveenvalecha
#8 interdiff-2870465-6-8.txt24.25 KBnaveenvalecha
#8 user-2870465-8.patch38.99 KBnaveenvalecha
#6 2870465-6.patch26.04 KBnaveenvalecha
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

michielnugter created an issue. See original summary.

michielnugter’s picture

Dropping a few dependencies and moving these tests out of scope. Keeping views because of the large amount of views tests.

michielnugter’s picture

Component: phpunit » user.module
Issue tags: +phpunit initiative
naveenvalecha’s picture

Status: Postponed » Needs review
FileSize
26.04 KB

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

//Naveen

Status: Needs review » Needs work

The last submitted patch, 6: 2870465-6.patch, failed testing. View results

naveenvalecha’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
38.99 KB
24.25 KB

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

Status: Needs review » Needs work

The last submitted patch, 8: user-2870465-8.patch, failed testing. View results

naveenvalecha’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
38.65 KB
4.9 KB

Here are the fixes of the failures.

//Naveen

naveenvalecha’s picture

Issue summary: View changes

Added followup issue link

Status: Needs review » Needs work

The last submitted patch, 10: 2870465-10.patch, failed testing. View results

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
39.17 KB
1.08 KB

Addressed #10 failures.

Lendude’s picture

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)

naveenvalecha’s picture

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

Lendude’s picture

Status: Needs review » Needs work

#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

naveenvalecha’s picture

#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

naveenvalecha’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
38.56 KB
622 bytes

#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

naveenvalecha’s picture

Issue summary: View changes
FileSize
31.62 KB
6.94 KB

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

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

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

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
  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).

Lendude’s picture

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)

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.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
30.57 KB
1.39 KB

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.

Status: Needs review » Needs work

The last submitted patch, 24: 2870465-24.patch, failed testing. View results

nlisgo’s picture

Assigned: Unassigned » nlisgo
nlisgo’s picture

Assigned: nlisgo » Unassigned
Lendude’s picture

Lendude’s picture

Status: Needs work » Needs review

Bleh, comment got eaten.

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

Status: Needs review » Needs work

The last submitted patch, 28: 2870465-28.patch, failed testing. View results

Lendude’s picture

Status: Needs work » Needs review
FileSize
1.33 KB
32.38 KB

How did this come back green before?

Oh well, fixed fails.

vaplas’s picture

Looks nice! All changes seem very certain, except few nits:

  1. +++ b/core/modules/user/tests/src/Functional/UserAdminListingTest.php
    @@ -62,20 +62,24 @@ public function testUserListing() {
    -    foreach ($result as $account) {
    -      $name = (string) $account->td[0]->span;
    +    foreach ($result as $key => $account) {
    +      $xpath_key = $key + 1;
    +      $account_columns = $this->xpath("//table/tbody/tr[$xpath_key]/td");
    +      $name = $account_columns[0]->getText();
           $roles = [];
    -      if (isset($account->td[2]->div->ul)) {
    -        foreach ($account->td[2]->div->ul->li as $element) {
    -          $roles[] = (string) $element;
    +      $account_roles = $this->xpath("//table/tbody/tr[$xpath_key]/td/div/ul/li");
    +      if (!empty($account_roles)) {
    +        foreach ($account_roles as $element) {
    +          $roles[] = $element->getText();
             }
           }
    +
           $result_accounts[$name] = [
             'name' => $name,
    -        'status' => (string) $account->td[1],
    +        'status' => $account_columns[1]->getHtml(),
             'roles' => $roles,
    -        'member_for' => (string) $account->td[3],
    -        'last_access' => (string) $account->td[4],
    +        'member_for' => $account_columns[3]->getHtml(),
    +        'last_access' => $account_columns[4]->getHtml(),
           ];
         }
    

    Looks like we can reduce different with orignal implementation. Example:

     # not use twice global path
    - $account_columns = $this->xpath("//table/tbody/tr[$xpath_key]/td");
    + $account_columns = $account->findAll('css', 'td');
    
     # check a specific column (at least for BC)
    - $account_roles = $this->xpath("//table/tbody/tr[$xpath_key]/td/div/ul/li");
    + $account_roles = $account_columns[2]->findAll('css', 'td div ul li');
    
     # use text to avoid html tags in data
    -        'status' => $account_columns[1]->getHtml(),
    +        'status' => $account_columns[1]->getText(),
    ...
    -        'member_for' => $account_columns[3]->getHtml(),
    -        'last_access' => $account_columns[4]->getHtml(),
    +        'member_for' => $account_columns[3]->getText(),
    +        'last_access' => $account_columns[4]->getText(),
    
  2. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -66,12 +71,14 @@ public function testUserAdmin() {
    -    $this->assertEqual($user_a->getUsername(), (string) $result[0]->td[1]->span, 'Filter by username returned the right user.');
    +    $span = $this->xpath('//table/tbody/tr/td[2]/span');
    +    $this->assertEqual($user_a->getUsername(), $span[0]->getText(), 'Filter by username returned the right user.');
    ...
    -    $this->assertEqual($user_a->getUsername(), (string) $result[0]->td[1]->span, 'Filter by username returned the right user.');
    +    $span2 = $this->xpath('//table/tbody/tr/td[2]/span');
    +    $this->assertEqual($user_a->getUsername(), $span2[0]->getText(), 'Filter by username returned the right user.');
    

    Similarly, save one-line, like:

    +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -66,12 +71,12 @@ public function testUserAdmin() {
    -    $this->assertEqual($user_a->getUsername(), (string) $result[0]->td[1]->span, 'Filter by username returned the right user.');
    +    $this->assertEqual($user_a->getUsername(), $result[0]->find('xpath', '/td[2]/span')->getText(), 'Filter by username returned the right user.');
    ...
    -    $this->assertEqual($user_a->getUsername(), (string) $result[0]->td[1]->span, 'Filter by username returned the right user.');
    +    $this->assertEqual($user_a->getUsername(), $result[0]->find('xpath', '/td[2]/span')->getText(), 'Filter by username returned the right user.');
    
  3. +++ b/core/modules/user/tests/src/Functional/UserLanguageCreationTest.php
    @@ -87,10 +87,10 @@ public function testLocalUserCreation() {
         // Set pass_raw so we can log in the new user.
    -    $user->pass_raw = $this->randomMachineName(10);
    +    $user->passRaw = $this->randomMachineName(10);
    

    Nit: pass_raw still in comments. Fixed :)

  4. +++ b/core/modules/user/tests/src/Functional/UserCancelTest.php
    @@ -99,23 +99,6 @@ public function testUserCancelUid1() {
    -    // Update uid 1's name and password to we know it.
    -    $password = user_password();
    -    $account = [
    -      'name' => 'user1',
    -      'pass' => $this->container->get('password')->hash(trim($password)),
    -    ];
    -    // We cannot use $account->save() here, because this would result in the
    -    // password being hashed again.
    -    db_update('users_field_data')
    -      ->fields($account)
    -      ->condition('uid', 1)
    -      ->execute();
    -
    -    // Reload and log in uid 1.
    -    $user_storage->resetCache([1]);
    -    $user1 = $user_storage->load(1);
    -    $user1->pass_raw = $password;
    
    @@ -127,7 +110,6 @@ public function testUserCancelUid1() {
    -    $user_storage->resetCache([1]);
    

    And this is the most suspicious change in my patch). It all started with the fact that I accidentally noticed that the testUserCancelUid1 green, although it still has $user1->pass_raw = $password; instead of passRaw.

    Suspicions intensified, after the question from @xjm in #2322195-157: Replace all instances of user_load(), user_load_multiple(), entity_load('user') and entity_load_multiple('user') with static method calls:

    Why are these resetCache() call necessary?

    After removed lines with resetCache() test still green.

    An expanded question appeared: why do we have to do something with user 1 (set a password, log in)? I did not find the answer and removed all the related code :) But if it goes beyond the scope, I will not argue.

Lendude’s picture

Status: Needs review » Needs work

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

vaplas’s picture

Hah, I did not even notice that it was no longer a simpletest. Because just finding other cases with 'pass_raw'. Reverted and filed #2926090: Remove dead code from UserCancelTest::testUserCancelUid1().

Lendude’s picture

Status: Needs work » Needs review

bump

vaplas’s picture

Nit clean up after #34.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

Status: Reviewed & tested by the community » Fixed

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

vaplas’s picture

#38: Bad object id: 6648bb6 :(

Also i checked 8.4 branch, and this patch needs to reroll, because UserPermissionsTest::testAccessContentPermission() added only for 8.5.x (#2892821: Core modules check node module's "access content" permission for displaying things that have nothing to do with nodes). Done.

vaplas’s picture

Status: Fixed » Needs review
vaplas’s picture

FileSize
1.42 KB

  • catch committed ff69088 on 8.5.x
    Issue #2870465 by naveenvalecha, vaplas, Lendude, Jo Fitzgerald,...
dawehner’s picture

Status: Needs review » Fixed

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

I just thought that additional synchronization between the branches would not hurt. But only for 8.5 - works for me too. Thanks!

dawehner’s picture

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

Lendude’s picture

@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*

dawehner’s picture

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.