Clean-up user module test members - ensure property definition and use of camelCase naming convention

Initial attempt. I removed a couple of properties in UserPictureTest which were not being used anywhere. I know it may not strictly be in the scope of the issue, but the properties would have to be renamed anyway, and since they are not used, it's better they are removed.

That was quick. Rerolling.

It seems I missed a typo.

Some more work done on this; let's see what the testbot thinks.

The patch in #6 refactors class-level properties from under_score to camelCase as the coding standards demand.

I know this because I applied the patch, ran my own coding standards review with netbeansdrupalcomposed, and poked through all the test classes to find camel case and underscore violations.

1. +++ b/core/modules/user/src/Tests/UserCancelTest.php
   @@ -25,6 +25,13 @@ class UserCancelTest extends WebTestBase {
   +  /**
   +   * A user with the 'administer users' permission.
   +   *
   +   * @var \Drupal\user\UserInterface
   +   */
   +  protected $adminUser;
   
   @@ -90,8 +97,8 @@ function testUserCancelUid1() {
   -    $this->admin_user = $this->drupalCreateUser(array('administer users'));
   -    $this->drupalLogin($this->admin_user);
   +    $this->adminUser = $this->drupalCreateUser(array('administer users'));
   +    $this->drupalLogin($this->adminUser);
   

   This does not need to be a class property - could just be $admin_user since it is only used in a single method and not created by setUp().

2. +++ b/core/modules/user/src/Tests/Views/AccessPermissionTest.php
   @@ -19,9 +19,9 @@
      /**
   -   * Views used by this test.
   +   * The names of the views used by this test.
       *
   -   * @var array
   +   * @var string[]
       */
      public static $testViews = array('test_access_perm');
   
   +++ b/core/modules/user/src/Tests/Views/AccessRoleTest.php
   @@ -21,9 +21,9 @@
      /**
   -   * Views used by this test.
   +   * The names of the views used by this test.
       *
   -   * @var array
   +   * @var string[]
       */
      public static $testViews = array('test_access_role');
   
   +++ b/core/modules/user/src/Tests/Views/AccessRoleUITest.php
   @@ -19,9 +19,9 @@
      /**
   -   * Views used by this test.
   +   * The names of the views used by this test.
       *
   -   * @var array
   +   * @var string[]
       */
      public static $testViews = array('test_access_role');
   
   +++ b/core/modules/user/src/Tests/Views/ArgumentDefaultTest.php
   @@ -17,9 +17,9 @@
      /**
   -   * Views used by this test.
   +   * The names of the views used by this test.
       *
   -   * @var array
   +   * @var string[]
       */
      public static $testViews = array('test_plugin_argument_default_current_user');
   
   +++ b/core/modules/user/src/Tests/Views/BulkFormTest.php
   @@ -25,9 +25,9 @@ class BulkFormTest extends UserTestBase {
      /**
   -   * Views used by this test.
   +   * The names of the views used by this test.
       *
   -   * @var array
   +   * @var string[]
       */
      public static $testViews = array('test_user_bulk_form');
   
   +++ b/core/modules/user/src/Tests/Views/HandlerArgumentUserUidTest.php
   @@ -17,9 +17,9 @@
      /**
   -   * Views used by this test.
   +   * The names of the views used by this test.
       *
   -   * @var array
   +   * @var string[]
       */
      public static $testViews = array('test_user_uid_argument');
   
   +++ b/core/modules/user/src/Tests/Views/HandlerFieldPermissionTest.php
   @@ -20,9 +20,9 @@
      /**
   -   * Views used by this test.
   +   * The names of the views used by this test.
       *
   -   * @var array
   +   * @var string[]
       */
      public static $testViews = array('test_field_permission');
   
   +++ b/core/modules/user/src/Tests/Views/HandlerFieldRoleTest.php
   @@ -18,9 +18,9 @@
      /**
   -   * Views used by this test.
   +   * The names of the views used by this test.
       *
   -   * @var array
   +   * @var string[]
       */
      public static $testViews = array('test_views_handler_field_role');
   
   +++ b/core/modules/user/src/Tests/Views/HandlerFieldUserNameTest.php
   @@ -18,9 +18,9 @@
      /**
   -   * Views used by this test.
   +   * The names of the views used by this test.
       *
   -   * @var array
   +   * @var string[]
       */
      public static $testViews = array('test_views_handler_field_user_name');
   
   +++ b/core/modules/user/src/Tests/Views/HandlerFilterPermissionTest.php
   @@ -20,9 +20,9 @@
      /**
   -   * Views used by this test.
   +   * The names of the views used by this test.
       *
   -   * @var array
   +   * @var string[]
       */
      public static $testViews = array('test_filter_permission');
   
   +++ b/core/modules/user/src/Tests/Views/HandlerFilterUserNameTest.php
   @@ -27,23 +27,23 @@ class HandlerFilterUserNameTest extends ViewTestBase {
      /**
   -   * Views used by this test.
   +   * The names of the views used by this test.
       *
   -   * @var array
   +   * @var string[]
       */
      public static $testViews = array('test_user_name');
   
   +++ b/core/modules/user/src/Tests/Views/RelationshipRepresentativeNodeTest.php
   @@ -17,9 +17,9 @@
      /**
   -   * Views used by this test.
   +   * Tha names of the views used by this test.
       *
   -   * @var array
   +   * @var string[]
       */
      public static $testViews = array('test_groupwise_user');
   
   +++ b/core/modules/user/src/Tests/Views/UserChangedTest.php
   @@ -25,9 +25,9 @@ class UserChangedTest extends ViewTestBase {
      /**
   -   * Views used by this test.
   +   * The names of the views used by this test.
       *
   -   * @var array
   +   * @var string[]
       */
      public static $testViews = array('test_user_changed');
   
   +++ b/core/modules/user/src/Tests/Views/UserDataTest.php
   @@ -25,9 +25,9 @@ class UserDataTest extends UserTestBase {
      /**
   -   * Views used by this test.
   +   * The names of the views used by this test.
       *
   -   * @var array
   +   * @var string[]
       */
      public static $testViews = array('test_user_data');
   

   These changes look unnecessary to me.

3. +++ b/core/modules/user/src/Tests/Views/ArgumentValidateTest.php
   @@ -17,13 +17,22 @@
   +  /**
   +   * The names of the views used by this test.
   +   *
   +   * @var string[]
   +   */
   +  public static $testViews = array(
   +    'test_view_argument_validate_user',
   +    'test_view_argument_validate_username'
   +  );
   

   Let's make this like all the others and put the views on one line.

Altered the change as #8 suggested

Looks like its still missing some correction

$ egrep -rl '\$this\->[a-z]+_[a-z_]+' core/modules/*/src/Tests/ | grep -i user
core/modules/comment/src/Tests/Views/ArgumentUserUIDTest.php
core/modules/comment/src/Tests/Views/FilterUserUIDTest.php
<strong>core/modules/user/src/Tests/UserCancelTest.php</strong>

The first two are fixed as part of #2380023: Clean-up Comment module Test members - ensure property definition and use of camelCase naming convention and are outside the scope of this issue.

With respect to the last file, however, you are correct. The patch at #8 reverted the addition of the $adminUser class property, but did not revert the doc block, and did not change the instances of $this->admin_user that appear in testUserCancelUid1().

Steps needed to fix:

• remove the docblock at lines 28:32
• change references in testUserCancelUid1() to $this->admin_user to simply $admin_user. (This is consistent with how it is done in other testing functions there.)

@tibbsa okay I will consider them. But I thought these changes were missing in the last patch.

diff --git a/core/modules/user/src/Tests/UserCancelTest.php b/core/modules/user/src/Tests/UserCancelTest.php
index 70a0448..2c71de5 100644
--- a/core/modules/user/src/Tests/UserCancelTest.php
+++ b/core/modules/user/src/Tests/UserCancelTest.php
@@ -160,11 +160,11 @@ function testUserBlock() {
     \Drupal::config('user.settings')->set('cancel_method', 'user_cancel_block')->save();
 
     // Create a user.
-    $web_user = $this->drupalCreateUser(array('cancel account'));
-    $this->drupalLogin($web_user);
+    $webUser = $this->drupalCreateUser(array('cancel account'));
+    $this->drupalLogin($webUser);
 
     // Load real user object.
-    $account = user_load($web_user->id(), TRUE);
+    $account = user_load($webUser->id(), TRUE);
 
     // Attempt to cancel account.
     $this->drupalGet('user/' . $account->id() . '/edit');
@@ -387,8 +387,8 @@ function testUserCancelByAdmin() {
     $account = $this->drupalCreateUser(array());
 
     // Create administrative user.
-    $admin_user = $this->drupalCreateUser(array('administer users'));
-    $this->drupalLogin($admin_user);
+    $adminUser = $this->drupalCreateUser(array('administer users'));
+    $this->drupalLogin($adminUser);
 
     // Delete regular user.
     $this->drupalGet('user/' . $account->id() . '/edit');
@@ -415,8 +415,8 @@ function testUserWithoutEmailCancelByAdmin() {
     $account->save();
 
     // Create administrative user.
-    $admin_user = $this->drupalCreateUser(array('administer users'));
-    $this->drupalLogin($admin_user);
+    $adminUser = $this->drupalCreateUser(array('administer users'));
+    $this->drupalLogin($adminUser);
 
     // Delete regular user without email address.
     $this->drupalGet('user/' . $account->id() . '/edit');
@@ -440,8 +440,8 @@ function testMassUserCancelByAdmin() {
     \Drupal::config('user.settings')->set('notify.status_canceled', TRUE)->save();
 
     // Create administrative user.
-    $admin_user = $this->drupalCreateUser(array('administer users'));
-    $this->drupalLogin($admin_user);
+    $adminUser = $this->drupalCreateUser(array('administer users'));
+    $this->drupalLogin($adminUser);
 
     // Create some users.
     $users = array();
@@ -473,8 +473,8 @@ function testMassUserCancelByAdmin() {
 
     // Ensure that admin account was not cancelled.
     $this->assertText(t('A confirmation request to cancel your account has been sent to your email address.'), 'Account cancellation request mailed message displayed.');
-    $admin_user = user_load($admin_user->id());
-    $this->assertTrue($admin_user->isActive(), 'Administrative user is found in the database and enabled.');
+    $adminUser = user_load($adminUser->id());
+    $this->assertTrue($adminUser->isActive(), 'Administrative user is found in the database and enabled.');
 
     // Verify that uid 1's account was not cancelled.
     $user1 = user_load(1, TRUE);

Local variables like $web_user, when used only within a particular function, do not follow the camelCase naming convention: they should use the underscore_separating convention. See Naming Convention: Functions and Variables in the coding standards.

Class-level properties (variables that are declared within a class but not within a particular function) are to use the camelCasing convention instead. See Naming Conventions (point #2) in the 'object-oriented' coding standards.

I have no idea why the decision was made to do this differently, except that it provides some hint in object-oriented code as to whether a variable (at least a variable with more than one word in its name) is local to a function or not. The fact that this remains ambiguous for one-word variable names would seem to undermine that logic, but I digress. The coding standards say what the coding standards say. :)

@alexpott's point about $adminUser was that it was only ever used within testUserCancelUid1(). As a result, there seemed to be no point in making it a class-level property, since no other function ever refers to $this->adminUser anyway, and as such it can simply remain a local variable ($admin_user).

@tibbsa thanks for the suggestions. I have changed the code as per your suggestion in #11

The patch doesn't apply, according to the testbot.

It was a straight reroll.

Patch does not covers the changes asked by alexpott in #8.2

New patch based on all suggestions.

hmm, trying once more.

@tadityar test them locally at first. #21, #23 have same failing tests.

Fixed for sure now.. at least in my localhost

the mistake was from drupalLogin($this->admin_user)

Needs reroll.

Sigh.

Great, thanks @cilefen.

A couple things I suppose are artifacts of reroll after reroll...

Unused property (as far as I can tell).

class UserCancelTest extends WebTestBase {
  protected $admin_user;

This one is declared and then replaced with a local variable:

class UserRolesAssignmentTest extends WebTestBase {
  protected $admin_user;

egrep -rl '\$this\->[a-z]+_[a-z_]+' core/modules/user/src/Tests/ is clean, and code looks alright. I was about to RTBC and then I saw some faults in coding standards (comment less than 80 characters).

I'll set it to RTBC. :-)

Patch applies, fixes camel case/underscore problem for the tests. Explicitly adds undeclared properties. Also fixes some documentation errors.

Committed 353a160 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to issue summary.