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

Initial attempt.

Found an underscore in Drupal\views\Tests\Handler\FieldWebTest:

  protected $column_map = array(
    'views_test_data_name' => 'name',
  );

1 out of 104 files isn't bad. :-)

Also made a video about how I did this review: http://youtu.be/H6pWz_6XoiQ :-)

That video ought to be added to the Contributor task: Improve patch coding standards and documentation if you ask me.

One has to wonder what purpose this variable serves if this test passed with the wrong variable name in the first place, but alas...

diff --git a/core/modules/views/src/Tests/Handler/FieldWebTest.php b/core/modules/views/src/Tests/Handler/FieldWebTest.php
index cd11a52..ceb8881 100644
--- a/core/modules/views/src/Tests/Handler/FieldWebTest.php
+++ b/core/modules/views/src/Tests/Handler/FieldWebTest.php
@@ -32,7 +32,12 @@ class FieldWebTest extends HandlerTestBase {
    */
   public static $modules = ['node'];

-  protected $column_map = array(
+  /**
+   * Maps between the key in the expected result and the query result.
+   *
+   * @var array
+   */
+  protected $columnMap = array(
     'views_test_data_name' => 'name',
   );

My video above was a bit misleading on the analysis. It looks like a common pattern in the views tests to have a $column_map property on all the test classes. I'm not sure what the analysis would be on whether to keep or toss the one you just changed above. And it's out of scope to make such a change anyway. :-)

phpcs says there aren't any under_score errors any more, so RTBC. Woot.

Patch in #6 passed test. Code looks good according to camel case coding standard. Moving this to RTBC.

Again with the fidgety testbot. RTBC from #7 for patch in #6 is renewed. Woot.

1. +++ b/core/modules/views/src/Tests/Handler/FieldUnitTest.php
   @@ -28,9 +28,14 @@ class FieldUnitTest extends ViewUnitTestBase {
   +  protected $columnMap = [
        'views_test_data_name' => 'name',
   -  );
   +  ];
   
   +++ b/core/modules/views/src/Tests/Handler/FieldWebTest.php
   @@ -32,7 +32,12 @@ class FieldWebTest extends HandlerTestBase {
   +  protected $columnMap = array(
        'views_test_data_name' => 'name',
      );
   
   +++ b/core/modules/views/src/Tests/Handler/FilterBooleanOperatorStringTest.php
   @@ -31,11 +31,16 @@ class FilterBooleanOperatorStringTest extends ViewUnitTestBase {
   +  protected $columnMap = [
        'views_test_data_id' => 'id',
   -  );
   +  ];
   
   +++ b/core/modules/views/src/Tests/Handler/FilterBooleanOperatorTest.php
   @@ -30,11 +30,16 @@ class FilterBooleanOperatorTest extends ViewUnitTestBase {
   +  protected $columnMap = [
        'views_test_data_id' => 'id',
   -  );
   +  ];
   
   +++ b/core/modules/views/src/Tests/Handler/FilterCombineTest.php
   @@ -22,12 +22,17 @@ class FilterCombineTest extends ViewUnitTestBase {
   +  protected $columnMap = [
        'views_test_data_name' => 'name',
        'views_test_data_job' => 'job',
   -  );
   +  ];
   
   +++ b/core/modules/views/src/Tests/Handler/FilterEqualityTest.php
   @@ -24,11 +24,16 @@ class FilterEqualityTest extends ViewUnitTestBase {
   +  protected $columnMap = [
        'views_test_data_name' => 'name',
   -  );
   +  ];
   
   +++ b/core/modules/views/src/Tests/Handler/FilterInOperatorTest.php
   @@ -24,9 +24,14 @@ class FilterInOperatorTest extends ViewUnitTestBase {
   +  protected $columnMap = array(
        'views_test_data_name' => 'name',
        'views_test_data_age' => 'age',
      );
   
   +++ b/core/modules/views/src/Tests/Handler/FilterNumericTest.php
   @@ -24,12 +24,17 @@ class FilterNumericTest extends ViewUnitTestBase {
   +  protected $columnMap = [
        'views_test_data_name' => 'name',
        'views_test_data_age' => 'age',
   -  );
   +  ];
   
   +++ b/core/modules/views/src/Tests/Handler/FilterStringTest.php
   @@ -24,11 +24,16 @@ class FilterStringTest extends ViewUnitTestBase {
   +  protected $columnMap = [
        'views_test_data_name' => 'name',
   -  );
   +  ];
   

   Sometimes there is a conversion to the new array format and sometimes there is not. How come? Changing to the new format is okay in this patch since these lines are changing but let's be consistent.

2. +++ b/core/modules/views/src/Tests/Handler/FilterInOperatorTest.php
   @@ -24,9 +24,14 @@ class FilterInOperatorTest extends ViewUnitTestBase {
   -  public static $testViews = array('test_view');
   +  public static $testViews = ['test_view'];
   
   +++ b/core/modules/views/src/Tests/Handler/FilterNumericTest.php
   @@ -24,12 +24,17 @@ class FilterNumericTest extends ViewUnitTestBase {
   -  public static $testViews = array('test_view');
   +  public static $testViews = ['test_view'];
   
   +++ b/core/modules/views/src/Tests/Handler/FilterStringTest.php
   @@ -24,11 +24,16 @@ class FilterStringTest extends ViewUnitTestBase {
   -  public static $testViews = array('test_view');
   +  public static $testViews = ['test_view'];
   

   These are out-of-scope changes.

Applied suggested change from #15.

er.. forgot to add ;

This kind of array syntax sometimes fails, I guess something is wrong with testbot. alexpott has mentioned to keep the syntax consistent in #15, isn't it?

I think we should use array() everywhere else. Rest of the child issues have this same syntax. See cilefen's comment here #2394419: Clean-up file module test members - ensure property definition and use of camelCase naming convention (comment)

reverted array style change.

Reverting all array syntax changes and found one more variable without a declaration.

Tests pass and phpcs can't find any improper underscore names.

Thanks for removing the array changes.

clean_up_views_module-2385209-24.patch no longer applies.

error: patch failed: core/modules/views/src/Tests/Wizard/TaggedWithTest.php:23
error: core/modules/views/src/Tests/Wizard/TaggedWithTest.php: patch does not apply

Thanks for the reroll.

I still can't find any class property underscore errors in the tests.

+++ b/core/modules/views/src/Tests/Plugin/AccessTest.php
@@ -33,6 +33,34 @@ class AccessTest extends PluginTestBase {
+  /**
+   * Role id of a web user.
+   *
+   * @var string
+   */
+  protected $webRole;
...
+  /**
+   * Role id of a normal user.
+   *
+   * @var string
+   */
+  protected $normalRole;

Not necessary to by class properties since afaics they are not used. We should still add the role to the user.

I would have marked it RTBC but there are some commented code in ViewStorageTest.php, it can be out of scope. The code looks ok and egrep -rl '\$this\->[a-z]+_[a-z_]+' core/modules/views/src/Tests/ returned no output.

Just added this for the commented code: #2413217: ViewStorageTest::displayMethodTests() needs @todo love.

I think we can agree this is RTBC. :-)

Then its okay. RTBC +1 :)

1. +++ b/core/modules/views/src/Tests/DefaultViewsTest.php
   @@ -40,13 +40,20 @@ class DefaultViewsTest extends ViewTestBase {
   +  /**
   +   * Name of a field for testing.
   +   *
   +   * @var string
   +   */
   +  protected $fieldName;
   

   Does not need to be a property.

2. +++ b/core/modules/views/src/Tests/Plugin/RelationshipJoinTestBase.php
   @@ -25,9 +25,9 @@
      /**
   -   * @var \Drupal\user\Entity\User
   +   * @var \Drupal\user\UserInterface
       */
   -  protected $root_user;
   +  protected $rootUser;
   

   Does not need to be a property.

@ardnet: I think there was some confusion. @alexpott didn't say to revert those property names like you have done in the interdiff. The properties are apparently not necessary at all and you can remove them, and their usage can be changed to local variable assignments.

Assigning to myself

Attaching a patch as per @Alex's comment #37.

1. +++ b/core/modules/views/src/Tests/DefaultViewsTest.php
   @@ -55,25 +55,25 @@ protected function setUp() {
        // Create a field.
   -    $this->field_name = Unicode::strtolower($this->randomMachineName());
   +    $fieldName = Unicode::strtolower($this->randomMachineName());
   

   +1 on making it a local variable, but -1 on leaving it as camelCase. It should be underscored, like this: $field_name

2. +++ b/core/modules/views/src/Tests/Plugin/RelationshipJoinTestBase.php
   @@ -39,8 +34,8 @@ protected function setUpFixtures() {
        // Create a record for uid 1.
        $this->installSchema('system', 'sequences');
   -    $this->root_user = entity_create('user', array('name' => $this->randomMachineName()));
   -    $this->root_user->save();
   +    $rootUser = entity_create('user', array('name' => $this->randomMachineName()));
   +    $rootUser->save();
   

   Same deal here. Change $rootUser to $root_user.

Done!

Great. :-)

And I thought this would be a reroll. :)

I am not sure if this is a random fail but it is strange that it passed earlier and suddenly started failing. Retesting it. Should go back to RTBC as per #52.

Committed 23a2db2 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

Cool!