The tracker 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
#10 2385217-10.patch4.92 KBrpayanm
#10 2385217-interdiff.txt1.71 KBrpayanm
#7 2385217-7.patch6.4 KBrpayanm
#7 2385217-interdiff.txt465 bytesrpayanm
#4 clean_up_tracker_module-2385217-4.patch6.35 KBhussainweb
#4 interdiff-1-4.txt948 byteshussainweb
#1 clean_up_tracker_module-2385217-1.patch5.66 KBareke
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

areke’s picture

areke CreditAttribution: areke commented
Assigned: Unassigned » areke
Status: Active » Needs review
FileSize
clean_up_tracker_module-2385217-1.patch5.66 KB
perennial.sky’s picture

perennial.sky CreditAttribution: perennial.sky commented

Looks good to me.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Needs review » Needs work

$this->comment in TrackerTestBase is not declared or documented.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
FileSize
interdiff-1-4.txt948 bytes
clean_up_tracker_module-2385217-4.patch6.35 KB

Fixed as per #3.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
Status: Needs work » Needs review

Forgot to set to needs review.

tibbsa’s picture

tibbsa CreditAttribution: tibbsa commented
Assigned: areke » Unassigned
Status: Needs review » Needs work

My only nit-pick is in TrackerTest.php; suggest:

   /**
    * A second user that will 'create' comments and nodes.
    *
-   * @var object
+   * @var \Drupal\user\UserInterface
    */

Other than that, this should be good to go.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm commented
Status: Needs work » Needs review
FileSize
2385217-interdiff.txt465 bytes
2385217-7.patch6.4 KB
tibbsa’s picture

tibbsa CreditAttribution: tibbsa commented
Status: Needs review » Reviewed & tested by the community

I did not find any other missing variables or docblock issues; setting RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/tracker/src/Tests/TrackerTest.php
@@ -119,8 +119,8 @@ function testTrackerUser() {
-    $admin_user = $this->drupalCreateUser(array('post comments', 'administer comments', 'access user profiles'));
-    $this->drupalLogin($admin_user);
+    $adminUser = $this->drupalCreateUser(array('post comments', 'administer comments', 'access user profiles'));
+    $this->drupalLogin($adminUser);

This change is incorrect $admin_user is not a class property.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm commented
Status: Needs work » Needs review
FileSize
2385217-interdiff.txt1.71 KB
2385217-10.patch4.92 KB

A question:

On core/modules/config/src/Tests/ConfigLanguageOverrideWebTest.php we have this code:

$adminUser = $this->drupalCreateUser(array('administer site configuration', 'administer languages'));
$this->drupalLogin($adminUser);

We might change to this:

$admin_user = $this->drupalCreateUser(array('administer site configuration', 'administer languages'));
$this->drupalLogin($admin_user);

???

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

Patch applies, phpcs says there are no inappropriate underscore property names, and addresses the issues of #3, #6, and #9.

Let's check the testbot's reaction since the last run is a little stale.

Mile23 queued 10: 2385217-10.patch for re-testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs review » Reviewed & tested by the community

Testbot believes.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2385217-10.patch, failed testing.

Status: Needs work » Needs review

rpayanm queued 10: 2385217-10.patch for re-testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs review » Reviewed & tested by the community

Still passes.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 7aad4bc and pushed to 8.0.x. Thanks!

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

  • alexpott committed 7aad4bc on 8.0.x 
    Issue #2385217 by rpayanm, hussainweb, areke: Clean-up tracker module...

Status: Fixed » Closed (fixed)

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