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

simple cleanup

  function testSearchResultsCommentAccess() {
    $comment_body = 'Test comment body';
    $this->comment_subject = 'Test comment subject';
    $roles = $this->adminUser->getRoles();
    $this->admin_role = $roles[0];

What about $this->admin_role and $this->comment_subject ? These should also be defined.

There are more instances in many files that need fixing.

$ egrep -rl '\$this\->[a-z]+_[a-z_]+' core/modules/search/src/Tests/
core/modules/search/src/Tests//SearchCommentCountToggleTest.php
core/modules/search/src/Tests//SearchCommentTest.php
core/modules/search/src/Tests//SearchConfigSettingsFormTest.php
core/modules/search/src/Tests//SearchEmbedFormTest.php
core/modules/search/src/Tests//SearchKeywordsConditionsTest.php
core/modules/search/src/Tests//SearchLanguageTest.php
core/modules/search/src/Tests//SearchMultilingualEntityTest.php
core/modules/search/src/Tests//SearchNodePunctuationTest.php
core/modules/search/src/Tests//SearchNodeUpdateAndDeletionTest.php
core/modules/search/src/Tests//SearchNumberMatchingTest.php
core/modules/search/src/Tests//SearchNumbersTest.php
core/modules/search/src/Tests//SearchPageCacheTagsTest.php
core/modules/search/src/Tests//SearchPageOverrideTest.php
core/modules/search/src/Tests//SearchPageTextTest.php

Good job. There are still some areas that need more fine-tuning though.

SearchCommentCountToggleTest.php

  /**
   * A user with permission to search and post comments.
   *
   * @var object
   */
  protected $searchingUser;

This isn't just an 'object' -- it should be a \Drupal\user\UserInterface.

SearchCommentTest.php (testSearchResultsCommentAccess())

$comment_body = 'Test comment body';
    $this->commentSubject = 'Test comment subject';
    $roles = $this->adminUser->getRoles();
    $this->adminRole = $roles[0];

$this->commentSubject and $this->adminRole are used without prior declaration.

Below are some other examples of properties being used without prior declaration (my proposed patch in #2383287: Add warning about use of undeclared class properties in test classes is helpful in spotting these, since you can run the tests locally to flag these).

Exception Other      SearchCommentTest  143 Drupal\search\Tests\SearchCommentTe
    Class property comment_subject implicitly created without prior declaration
Exception Other      SearchCommentTest  143 Drupal\search\Tests\SearchCommentTe
    Class property admin_role implicitly created without prior declaration
Exception Other      SearchCommentTest  143 Drupal\search\Tests\SearchCommentTe
    Class property node implicitly created without prior declaration
Exception Other      SearchKeywordsCon   40 Drupal\search\Tests\SearchKeywordsC
    Class property searching_user implicitly created without prior declaration
Exception Other      SearchLanguageTes   87 Drupal\search\Tests\SearchLanguageT
    Class property searchable_nodes implicitly created without prior declaration
Exception Other      SearchPreprocessL   38 Drupal\search\Tests\SearchPreproces
    Class property node implicitly created without prior declaration

(Note that many of these properties need not only a declaration but a rename to camelCase convention.)

Changes as per #9.

Thanks! I haven't checked to see if there are other variables needing declaration, but what is in this patch looks mostly good.

A few items to fix that I noticed:

a) SearchCountToggleTest

+   * @var array
+   */
+  protected $searchableNodes;

It is preferable to use @var \Drupal\something\something[] rather than @var array, because it is more specific and helps IDE users.

Also same problem in SearchLanguageTest, SearchMultilingualEntityTest, etc.

b) SearchCommentTest

+  /**
+   * Id for the administrator role.
+   *
+   * @var string
+   */
+  protected $adminRole;

"Id" is a psychological term. This should be "ID", which means identifier.

c) Hopefully not out of scope for this issue:

+++ b/core/modules/search/src/Tests/SearchEmbedFormTest.php
@@ -29,7 +29,7 @@ class SearchEmbedFormTest extends SearchTestBase {
   /**
    * Count of how many times the form has been submitted.
    */
-  public $submit_count = 0;
+  public $submitCount = 0;

This doc block could use a @var line (looks like it's an int).

d) SearchNodePunctuationTest

+  /**
+   * A user with permissions to use advanced search.
+   *
+   * @var object
+   */
+  public $testUser;
 

This one is still @var object instead of the class/interface. SearchPageCacheTagsTest too, same variable, and SearchPageOverrideTest, and several other classes.

Also probably "permissions" should be "permission"? Your patch has "permissions" in several places but "permission" in most; permission is better.

e)

+++ b/core/modules/search/src/Tests/SearchNumberMatchingTest.php
@@ -15,15 +15,20 @@
  * @group search
  */
 class SearchNumberMatchingTest extends SearchTestBase {
-  protected $test_user;
+  /**
+   * A user with permissions to administer nodes.
+   *
+   * @var object
+   */
+  protected $testUser;
   protected $numbers;
   protected $nodes;

Looks like $numbers and $nodes need doc blocks? Same in SearchNumbersTest.

Patch still applies.

This should resolve some more of the things from #11.

One potentially controversial suggested change arises in the number tests. Having moved the explanation of what the variable was to the class definition, which wouldn't be duplicated in the setUp() function body, it seemed to make sense to also move the list of numbers to be tested to be a part of the declaration itself (given that they are "set and never changed" anywhere else). That is not strictly a coding standards issue though.

Looks better! I think your changes in the Numbers tests make sense.

I only found one small thing to fix, in SearchEmbedFormTest:

+   *
+   * @var integer
    */
-  public $submit_count = 0;
+  protected $submitCount = 0;

In @var we use "int" not "integer". See https://www.drupal.org/node/1354#types

The rest looks great, thanks!

I also looked through all of the Search tests to see if there were any other things that should be fixed in this way, and I think it's all good now.

Although I did notice that SearchMatchTest has some const declarations in the global scope at the top of the file, that should probably be class members, and which do not have doc blocks:

// The search index can contain different types of content. Typically the type
// is 'node'. Here we test with _test_ and _test2_ as the type.
const SEARCH_TYPE = '_test_';
const SEARCH_TYPE_2 = '_test2_';
const SEARCH_TYPE_JPN = '_test3_';

I'm not sure if that is in scope for this issue or if we should file a separate issue? These could be documented as:

/**
 * Search type to test that index is separated by type.
 */

or something like that.

I'd say the constants are out of scope for this issue.

The @var part would be, too, but it's already in the patch.

Fair enough. Here's a new patch with the @var int changed to @var integer, which I did by editing the -13 patch file. It should be RTBC now.

Patch doesn't miss any under_score class properties, and passes tests, so RTBC +1.

Rolling.

Reroll, nothing fancy.

Evidently there was an API change on UserInterface::getRoles() in HEAD and I missed the added parameter during the re-roll. This should work now.

diff --git a/core/modules/search/src/Tests/SearchCommentTest.php b/core/modules/search/src/Tests/SearchCommentTest.php
index c15c1ff..1818350 100644
--- a/core/modules/search/src/Tests/SearchCommentTest.php
+++ b/core/modules/search/src/Tests/SearchCommentTest.php
@@ -169,7 +169,7 @@ function testSearchResultsComment() {
   function testSearchResultsCommentAccess() {
     $comment_body = 'Test comment body';
     $this->commentSubject = 'Test comment subject';
-    $roles = $this->adminUser->getRoles();
+    $roles = $this->adminUser->getRoles(TRUE);
     $this->adminRole = $roles[0];

     // Create a node.

phpcs gives you a green light, bringing back the RTBC.

+++ b/core/modules/search/src/Tests/SearchAdvancedSearchFormTest.php
@@ -13,6 +13,11 @@
+   * @var \Drupal\ndoe\NodeInterface

This should be "node" not "ndoe". Also while we're at it, let's add a blank line before the variable declaration.

The patch in #27 applies, changes relevant underscore to camelCase, and does not contain the 'ndoe' typo. :-)

Tests green locally against HEAD. Suspect Testbot brain fart. Will re-queue for testing in a little while once it hopefully gets past its hiccups.

All good, back to RTBC.

RTBC+1.

Committed 1241586 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.