The field 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

Mile23’s picture

Status: Active » Needs review
FileSize
32.33 KB

Removed DisplayApiTest::field_name because it's unused.

Other than that, I only changed under_score class properties to camelCase, without any other changes.

Status: Needs review » Needs work

The last submitted patch, 1: 2392669_1.patch, failed testing.

Status: Needs work » Needs review

Mile23 queued 1: 2392669_1.patch for re-testing.

subhojit777’s picture

  1. +++ b/core/modules/field/src/Tests/DisplayApiTest.php
    @@ -17,13 +17,6 @@
       /**
    -   * The field name to use in this test.
    -   *
    -   * @var string
    -   */
    -  protected $field_name;
    -
    -  /**
    

    Removed here.

  2. +++ b/core/modules/field/src/Tests/DisplayApiTest.php
    @@ -98,12 +91,12 @@ protected function setUp() {
    -      ->setComponent($this->field_name, $this->display_options['default'])
    +      ->setComponent($this->field_name, $this->displayOptions['default'])
    

    Hence, not changed in this place.

  3. +++ b/core/modules/field/src/Tests/ShapeItemTest.php
    @@ -29,20 +29,20 @@ class ShapeItemTest extends FieldUnitTestBase {
    -  protected $field_name = 'field_shape';
    +  protected $fieldName = 'field_shape';
    

    Not removed from here, but renamed.

  4. +++ b/core/modules/field/src/Tests/ShapeItemTest.php
    @@ -29,20 +29,20 @@ class ShapeItemTest extends FieldUnitTestBase {
    -      'field_name' => $this->field_name,
    +      'field_name' => $this->fieldName,
    

    Converted to camel case due to change.

Why the difference, (1-2) and (3-4).

Mile23’s picture

FileSize
34.07 KB
6.63 KB

Pretty sure I messed that one up. :-)

Let's see how this try goes.

Status: Needs review » Needs work

The last submitted patch, 5: 2392669_5.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
36.59 KB
6.03 KB

Woo NetBeans! Time to get rid of it!

Status: Needs review » Needs work

The last submitted patch, 7: 2392669_7.patch, failed testing.

Mile23’s picture

FileSize
36.53 KB

Needed a rebase.

Mile23’s picture

Status: Needs work » Needs review
subhojit777’s picture

FileSize
40.02 KB
3.77 KB

I did egrep -rl '\$this\->[a-z]+_[a-z_]+' core/modules/field/src/Tests/ and got the output:

core/modules/field/src/Tests/Boolean/BooleanFieldTest.php
core/modules/field/src/Tests/Email/EmailFieldTest.php
core/modules/field/src/Tests/FormTest.php
core/modules/field/src/Tests/reEnableModuleFieldTest.php
core/modules/field/src/Tests/FieldImportDeleteUninstallUiTest.php

There were only $this->web_user. I was looking into NumberFieldTest.php and there was this:

  /**
   * A user with permission to view and manage entities and content types.
   *
   * @var \Drupal\user\UserInterface
   */
  protected $webUser;

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

    $this->webUser = $this->drupalCreateUser(array('view test entity', 'administer entity_test content', 'administer content types', 'administer node fields', 'administer node display', 'bypass node access'));
    $this->drupalLogin($this->webUser);
  }

Therefore, I made this changes to others too. I think these changes will make sense.
Also I see there are some commented-out codes in FormTest.php. Do you think we should remove it?

Mile23’s picture

Status: Needs review » Needs work

The fact that you only changed the under_score ones in the interdiff means that those properties are probably not used anywhere else in the class. In that case, just turning them into local variables, or even factoring them away is completely appropriate.

For instance, something like this:

    $this->drupalLogin($this->drupalCreateUser(array(
       'view test entity',
       'administer entity_test content',
       'administer entity_test form display',
       'administer entity_test fields',
     ));

Because that login will persist after setUp() is done, and into the test method itself. If the test method needs the property, though, leave it.

I'd say commented code is strictly speaking out of scope for this issue, but if not now, when? :-)

subhojit777’s picture

@Mile23 +1
So shall we change it everywhere else I mean also change it in NumberFieldTest.php. I have checked there, it uses $webUser just for login purpose, making it a class property does not make sense.

subhojit777’s picture

As for the commented codes, I will check whether they can be marked as TODO.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
40 KB

Just rerolling for now.

hussainweb’s picture

Status: Needs review » Needs work

Back to NW as per #12.

Mile23’s picture

Thanks, @hussainweb for rerolling.

@subhojit777 The questions that remain are:

Should we refactor away unused properties? The answer is yes.

Should we mark commented code as @todo? I just made an issue about it: #2411961: Figure out what to do with Drupal\field\Tests\FormTest::testFieldFormMultiple() Add a link to that issue in the @todo.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
39.99 KB

Rerolling again... :)

Status: Needs review » Needs work

The last submitted patch, 18: clean_up_field_module-2392669-18.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
703 bytes
40.43 KB

Fixing the test. I think the other failure was random, something to do with a TWIG class not found.

subhojit777’s picture

subhojit777’s picture

Assigned: Unassigned » subhojit777
Status: Needs review » Needs work

Refactor unused properties:

  • protected $webUser; from BooleanFieldtest.php. Follow #12
  • protected $webUser; from NumberFieldTest.php. Follow #12
  • protected $webUser; from EmailFieldTest.php. Follow #12
  • protected $articleCreator; from reEnableModuleFieldTest.php. Follow #12
subhojit777’s picture

Assigned: subhojit777 » Unassigned
Status: Needs work » Needs review
FileSize
39.86 KB
3.88 KB
Mile23’s picture

Status: Needs review » Reviewed & tested by the community

I'm going to mark this as RTBC even though I have the first patch up there.

It solves the problem of underscore vs. camel case, the patch applies, and my next step is to have the testbot run it again.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/field/src/Tests/FieldImportDeleteUninstallUiTest.php
@@ -25,11 +25,18 @@ class FieldImportDeleteUninstallUiTest extends FieldTestBase {
+  /**
+   * A user with permission to synchronize configuration.
+   *
+   * @var \Drupal\user\UserInterface
+   */
+  protected $webUser;

Does not need to be a property.

AjitS’s picture

Assigned: Unassigned » AjitS
Issue tags: +Needs reroll

The patch no longer applies and needs a reroll.
Doing it now.

AjitS’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
39.89 KB

Let's see what the testbot says. After it clears, I will make the changes suggested in #26.

Status: Needs review » Needs work

The last submitted patch, 28: clean_up_field_module-2392669-28.patch, failed testing.

AjitS’s picture

Assigned: AjitS » Unassigned

Not sure why the test is failing.

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
39.81 KB

Re-rolled!

Status: Needs review » Needs work

The last submitted patch, 31: clean_up_field_module-2392669-31.patch, failed testing.

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
39.78 KB

Re-rolled and fixed broken test.

Status: Needs review » Needs work

The last submitted patch, 33: clean_up_field_module-2392669-33.patch, failed testing.

hussainweb’s picture

@rteijeiro: Might I suggest to include an interdiff for your fixes. I know you rerolled and an interdiff doesn't make sense there but in such situations, I post two different patches. The first just after rerolling and no changes, and second patch after the fixes where you can get an interdiff.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
553 bytes
40.07 KB

Fixing the exceptions.

hussainweb’s picture

I think the change requested in #26 got left out in middle of all the rerolls and failures. Doing that here.

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Looks great! Good job @hussainweb! Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/field/src/Tests/BulkDeleteTest.php
    @@ -50,7 +50,7 @@ class BulkDeleteTest extends FieldUnitTestBase {
        * @var array
        */
    -  protected $entity_type = 'entity_test';
    +  protected $entityType = 'entity_test';
    

    Let's rename this to $entityTypeId. Also it's not an array.

  2. +++ b/core/modules/field/src/Tests/TranslationWebTest.php
    @@ -31,14 +31,14 @@ class TranslationWebTest extends FieldTestBase {
    -  protected $entity_type = 'entity_test_mulrev';
    +  protected $entityType = 'entity_test_mulrev';
    

    As above $entityTypeId

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
39.92 KB
7.48 KB

Nice catch, @alexpott! Thanks!

hussainweb’s picture

Yay! @alexpott: Should be good for RTBC again?

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC?

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Test code is not frozen in beta. Committed 1f37397 and pushed to 8.0.x. Thanks!

  • alexpott committed 1f37397 on 8.0.x
    Issue #2392669 by hussainweb, Mile23, rteijeiro, subhojit777, AjitS:...

Status: Fixed » Closed (fixed)

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