The search module uses Test class members with underscored names. Some examples are big_user, web_user and admin_user, but there could be others. According to our coding conventions, these should be renamed to bigUser, webUser and adminUser. In addition, some properties are undefined but should be.

See the parent issue #1811638: [meta] Clean-up Test members - ensure property definition and use of camelCase naming convention.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, because this is a coding standards change.
Issue priority Not critical because coding standard changes are not critical.
Unfrozen changes Unfrozen because it only changes automated tests.
Disruption There is no disruption expected from this sort of change.
CommentFileSizeAuthor
#27 clean_up_search_module-2380429-27.patch35.63 KBtibbsa
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,313 pass(es). View
#27 clean_up_search_module-2380429-24-27-interdiff.txt566 bytestibbsa
#24 clean_up_search_module-2380429-24.patch35.63 KBtibbsa
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,178 pass(es). View
#22 clean_up_search_module-2380429-22.patch35.62 KBtibbsa
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,144 pass(es), 8 fail(s), and 0 exception(s). View
#17 interdiff.txt404 bytesjhodgdon
#17 clean_up_search_module-2380429-17.patch35.62 KBjhodgdon
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch clean_up_search_module-2380429-17.patch. Unable to apply patch. See the log in the details link for more information. View
#13 clean_up_search_module-2380429-13.patch35.62 KBtibbsa
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,164 pass(es). View
#13 clean_up_search_module-2380429-10-13-interdiff.txt11.62 KBtibbsa
#10 clean_up_search_module-2380429-10.patch31.93 KBhussainweb
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,101 pass(es). View
#10 interdiff-8-10.txt3.24 KBhussainweb
#8 clean_up_search_module-2380429-8.patch30.31 KBhussainweb
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,760 pass(es). View
#8 interdiff-1-8.txt28.92 KBhussainweb
#1 search.patch2.09 KBmarkat
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,440 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

markat’s picture

Status: Active » Needs review
FileSize
2.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,440 pass(es). View
pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

simple cleanup

cilefen’s picture

Issue summary: View changes
alexpott’s picture

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

cilefen’s picture

Issue summary: View changes
cilefen’s picture

Title: Clean-up search module test members to use camelCase naming convention » Clean-up search module test members - ensure property definition and use of camelCase naming convention
cilefen’s picture

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

Status: Needs work » Needs review
FileSize
28.92 KB
30.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,760 pass(es). View
tibbsa’s picture

Status: Needs review » Needs work

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

hussainweb’s picture

Status: Needs work » Needs review
FileSize
3.24 KB
31.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,101 pass(es). View

Changes as per #9.

jhodgdon’s picture

Status: Needs review » Needs work

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.

Mile23’s picture

Patch still applies.

tibbsa’s picture

Assigned: Unassigned » tibbsa
Status: Needs work » Needs review
FileSize
11.62 KB
35.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,164 pass(es). View

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.

tibbsa’s picture

Assigned: tibbsa » Unassigned
jhodgdon’s picture

Status: Needs review » Needs work

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.

Mile23’s picture

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.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
35.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch clean_up_search_module-2380429-17.patch. Unable to apply patch. See the log in the details link for more information. View
404 bytes

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.

Mile23’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: clean_up_search_module-2380429-17.patch, failed testing.

jhodgdon’s picture

Issue tags: +Needs reroll
tibbsa’s picture

Assigned: Unassigned » tibbsa

Rolling.

tibbsa’s picture

Status: Needs work » Needs review
FileSize
35.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,144 pass(es), 8 fail(s), and 0 exception(s). View

Reroll, nothing fancy.

Status: Needs review » Needs work

The last submitted patch, 22: clean_up_search_module-2380429-22.patch, failed testing.

tibbsa’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
35.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,178 pass(es). View

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

Status: Needs review » Reviewed & tested by the community

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

tstoeckler’s picture

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

tibbsa’s picture

Status: Needs work » Needs review
FileSize
566 bytes
35.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,313 pass(es). View
Mile23’s picture

Status: Needs review » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 27: clean_up_search_module-2380429-27.patch, failed testing.

tibbsa’s picture

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.

Status: Needs work » Needs review
tibbsa’s picture

Status: Needs review » Reviewed & tested by the community

All good, back to RTBC.

tibbsa’s picture

Mile23’s picture

RTBC+1.

tibbsa’s picture

Assigned: tibbsa » Unassigned
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1241586 and pushed to 8.0.x. Thanks!

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

  • alexpott committed 1241586 on 8.0.x
    Issue #2380429 by tibbsa, hussainweb, jhodgdon, markat: Clean-up search...

Status: Fixed » Closed (fixed)

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