The node 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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tibbsa’s picture

Changes to the node module.

I also noted a few instances where docblocks were not included for the $webUser variables, and a few other where the $webUser variable was not even declared as a protected member variable prior to use, and included insertions for those. While arguably out of scope for this change, this does not affect functionality in any way, and opening a separate issue for this change seemed overkill.

tibbsa’s picture

Status: Active » Needs review
FileSize
43.96 KB

Sending to the Testbot

tibbsa’s picture

Assigned: tibbsa » Unassigned
cilefen’s picture

Issue summary: View changes
tibbsa’s picture

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

Status: Needs review » Needs work

There are more instances to fix:

$ egrep -rl '\$this\->[a-z]+_' core/modules/node/src/Tests/
core/modules/node/src/Tests//MultiStepNodeFormBasicOptionsTest.php
core/modules/node/src/Tests//NodeAccessBaseTableTest.php
core/modules/node/src/Tests//NodeAdminTest.php
core/modules/node/src/Tests//NodeRevisionPermissionsTest.php
core/modules/node/src/Tests//PagePreviewTest.php
core/modules/node/src/Tests//Views/FrontPageTest.php
core/modules/node/src/Tests//Views/NodeFieldFilterTest.php
core/modules/node/src/Tests//Views/NodeLanguageTest.php
tibbsa’s picture

Status: Needs work » Needs review
FileSize
25.87 KB
68.21 KB

Views/FrontPageTest.php includes a reference to $this->root_user, correction of which will have to take place as part of the 'simpletest' module cleanup.

Mile23’s picture

Status: Needs review » Needs work

Just a few things:

NodeAccessFieldTest, line 80: Missed a camelCase.

NodeRevisionsAllTest: No @var docblocks on the properties.

NodeTranslationUITest: testDisabledBundle(), line 208: Missed a camelCase.

tibbsa’s picture

Somehow your line numbers aren't making sense with what I have in the code. Can you clarify what you're looking at that appears to be missing?

NodeRevisionsAllTest: No @var docblocks on the properties.
No properties were otherwise changed or added in this class as part of this issue. We did not necessarily go and document every missing docblock -- but if we were adding new properties, those got documented as we went.

Note the comment in the parent issue: "Do not, as part of this work, add documentation, variables, or otherwise refactor any code that you are otherwise not changing at all to accomplish the above three tasks. If you spot other problems, it is better to create a new issue for these."

NodeAccessFieldTest, line 80: Missed a camelCase.

[78] function testNodeAccessAdministerField() {
[79]     // Create a page node.
[80]     $fieldData = array();
[81]     $value = $fieldData[0]['value'] = $this->randomMachineName();
[82]     $node = $this->drupalCreateNode(array($this->fieldName => $fieldData));

NodeTranslationUITest: testDisabledBundle(), line 208: Missed a camelCase.

[206]  public function testDisabledBundle() {
[207]    // Create a bundle that does not have translation enabled.
[208]    $disabledBundle = $this->randomMachineName();
[209]    $this->drupalCreateContentType(array('type' => $disabledBundle, 'name' => $\
disabledBundle));

Status: Needs work » Needs review
Mile23’s picture

OK then. Let's re-test since the patch is stale, and then it's off to RTBC land if it's green.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

The patch in #7 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. I found a few that were mentioned in #9, but those are apparently out of scope for this issue.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/node/src/Tests/NodeAccessBaseTableTest.php
@@ -30,6 +30,22 @@ class NodeAccessBaseTableTest extends NodeTestBase {
+  /**
+   * An array of nids that are found to be visible while testing.
+   *
+   * @var array
+   */
+  protected $nidsVisible = array();

@@ -147,11 +162,11 @@ function testNodeAccessBasic() {
+      $this->nidsVisible = array();
...
+        $this->nidsVisible[$matches[1]] = TRUE;

@@ -163,10 +178,10 @@ protected function assertTaxonomyPage($is_admin) {
+          $this->assertIdentical(isset($this->nidsVisible[$nid]), $should_be_visible, strtr('A %private node by user %uid is %visible for user %current_uid on the %tid_is_private page.', array(
...
+            '%visible' => isset($this->nidsVisible[$nid]) ? 'visible' : 'not visible',

This does not need to be class properties - it is only used with the scope of a single method.

tibbsa’s picture

Status: Needs work » Needs review
FileSize
2.33 KB
68.05 KB
Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Patch in #14 applies, passes the testbot, changes property names from underscore to camel case, and refactors $this->nidsVisible to a local variable as per #13.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: clean_up_node-2380773-14.patch, failed testing.

Mile23’s picture

Previous fail says: "Setup environment The testbot client is probably malfunctioning."

Running again.

Status: Needs work » Needs review
Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: clean_up_node-2380773-14.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
68.05 KB

A straight reroll did the trick.

subhojit777’s picture

Well I am not sure about this:

In FrontPageTest.php:

  /**
   * Tests the frontpage when logged in as admin.
   */
  public function testAdminFrontPage() {
    // When a user with sufficient permissions is logged in, views_ui adds
    // contextual links to the homepage view. This verifies there are no errors.
    \Drupal::service('module_installer')->install(array('views_ui'));
    // Login root user with sufficient permissions.
    $this->drupalLogin($this->root_user); // << See here
    // Test frontpage view.
    $this->drupalGet('node');
    $this->assertResponse(200);
    // Check that the frontpage view was rendered.
    $this->assertPattern('/class=".+view-frontpage/', 'Frontpage view was rendered');
  }

In NodeAccessViewGrantsCacheContextTest.php:

  protected function setUp() {
    parent::setUp();

    node_access_rebuild();

    // Create some content.
    $this->drupalCreateNode();
    $this->drupalCreateNode();
    $this->drupalCreateNode();
    $this->drupalCreateNode();

    // Create user with simple node access permission. The 'node test view'
    // permission is implemented and granted by the node_access_test module.
    $this->accessUser = $this->drupalCreateUser(array('access content overview', 'access content', 'node test view'));
    $this->noAccessUser = $this->drupalCreateUser(array('access content overview', 'access content'));
    $this->noAccessUser2 = $this->drupalCreateUser(array('access content overview', 'access content'));

    $this->userMapping = [
      1 => $this->root_user, // << See here
      2 => $this->accessUser,
      3 => $this->noAccessUser,
    ];
  }

Find for << See here in above code snippets.
I have tried changing root_user to rootUser, but the test is failing. Am I missing something obvious?

hussainweb’s picture

@subhojit777: The root_user is setup in WebTestBase and will have to be handled by #2382195: Clean-up simpletest module test members - ensure property definition and use of camelCase naming convention. You can't change it in this patch since it requires changing WebTestBase.php.

subhojit777’s picture

Status: Needs review » Reviewed & tested by the community

1. Did egrep -rl '\$this\->[a-z]+_[a-z_]+' core/modules/node/src/Tests/, and got this as output:

core/modules/node/src/Tests/Views/FrontPageTest.php
core/modules/node/src/Tests/NodeAccessViewGrantsCacheContextTest.php

From @hussainweb's comment in #24, I guess there is no problem with that.

2. Test passed in #22

3. Did manual review of the patch in #21, it looks good.

Hence RTBC :)

  • webchick committed 8d3a5f7 on 8.0.x
    Issue #2380773 by tibbsa, hussainweb, Mile23: Clean-up Node module test...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks like Alex has been committing others of these, so joining the club. :P

Committed and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

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