Convert field tests to use ::$modules property instead of parent::setUp($modules) [#1711316]

Comment	File	Size	Author
#13	field-171131-13.patch	17.47 KB	kid_icarus

#13	interdiff.txt	2.28 KB	kid_icarus
#10	field-1711316-10.patch	17.89 KB	kid_icarus

#10	interdiff.txt	1.29 KB	kid_icarus
#6	field-1711316-6.patch	17.89 KB	kid_icarus

#6	interdiff.txt	1.52 KB	kid_icarus
#4	field-1711316-4.patch	17.7 KB	kid_icarus

#4	interdiff.txt	13.12 KB	kid_icarus
#1	drupal-1711316-1.patch	6.45 KB	kid_icarus

Comment #1

kid_icarus CreditAttribution: kid_icarus commented 3 August 2012 at 03:28

File	Size
drupal-1711316-1.patch	6.45 KB

I didn't change FieldAttachTestBase.php because things looked a little weird.

  function setUp() {
    $modules = func_get_args();
    if (isset($modules[0]) && is_array($modules[0])) {
      $modules = $modules[0];
    }
    if (!in_array('field_test', $modules)) {
      $modules[] = 'field_test';
    }
    parent::setUp($modules);

Same with FieldTestBase.php:

    $modules = func_get_args();
    if (isset($modules[0]) && is_array($modules[0])) {
      $modules = $modules[0];
    }
    parent::setUp($modules);

What of it?

Log in or register to post comments

Comment #2

kid_icarus CreditAttribution: kid_icarus commented 3 August 2012 at 03:29

Assigned:	Unassigned	» kid_icarus
Status:	Active	» Needs review

Log in or register to post comments

Comment #3

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 3 August 2012 at 03:31

Status:

Needs review

» Needs work

Hm, I'll have to look into that tomorrow when I'm fully awake.

Don't forget about the stuff in core/modules/field/modules/*/lib.

Log in or register to post comments

Comment #4

kid_icarus CreditAttribution: kid_icarus commented 4 August 2012 at 21:17

Status:

Needs work

» Needs review

File	Size
interdiff.txt	13.12 KB
field-1711316-4.patch	17.7 KB

So I think I got it right, but one thing I'm unsure about is removing the following chunk of documentation from FieldAttachTestBase and FieldTestBase

  function setUp() {
-    // Since this is a base class for many test cases, support the same
-    // flexibility that Drupal\simpletest\WebTestBase::setUp() has for the
-    // modules to be passed in as either an array or a variable number of string
-    // arguments.

Log in or register to post comments

Comment #5

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 6 August 2012 at 02:00

Status:

Needs review

» Needs work

+++ b/core/modules/field/lib/Drupal/field/Tests/CrudTest.phpundefined
@@ -11,6 +11,14 @@ use Drupal\field\FieldException;
 class CrudTest extends FieldTestBase {
+
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   */
+  public static $modules = array('field_test', 'number');
+
   public static function getInfo() {
     return array(
       'name' => 'Field CRUD tests',
@@ -19,11 +27,6 @@ class CrudTest extends FieldTestBase {

@@ -19,11 +27,6 @@ class CrudTest extends FieldTestBase {
     );
   }
 
-  function setUp() {
-    // field_update_field() tests use number.module
-    parent::setUp('field_test', 'number');
-  }
-
   // TODO : test creation with

This comment should be moved up and fixed for the $modules docblock.

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldTestBase.phpundefined
@@ -13,21 +13,22 @@ use Drupal\simpletest\WebTestBase;
 abstract class FieldTestBase extends WebTestBase {
+
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   */
+  public static $modules = array();
+
   var $default_storage = 'field_sql_storage';

I don't think there's any need to add this, it can be left out.

Log in or register to post comments

Comment #6

kid_icarus CreditAttribution: kid_icarus commented 6 August 2012 at 16:23

Status:

Needs work

» Needs review

File	Size
interdiff.txt	1.52 KB
field-1711316-6.patch	17.89 KB

Ok, here is my shot at correcting the docblock.

Log in or register to post comments

Comment #7

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 8 August 2012 at 17:13

Status:

Needs review

» Needs work

+++ b/core/modules/field/lib/Drupal/field/Tests/CrudTest.phpundefined
@@ -11,6 +11,20 @@ use Drupal\field\FieldException;
+   * @todo: Test creation with:
+   *   - A full fledged $field structure, check that all the values are there.
+   *   - A minimal $field structure, check all default values are set.
+   * @todo: Defer actual $field comparison to a helper function, used for the
+   *   two cases below.

This wasn't actually related to the setUP stuff, I think it was a call for a new test method.

+++ b/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Tests/FieldSqlStorageTest.phpundefined
@@ -28,7 +36,7 @@ class FieldSqlStorageTest extends WebTestBase {
+    parent::setUp();
     $this->field_name = strtolower($this->randomName());

Missing a blank line

+++ b/core/modules/field/modules/number/lib/Drupal/number/Tests/NumberFieldTest.phpundefined
@@ -26,7 +34,7 @@ class NumberFieldTest extends WebTestBase {
+    parent::setUp();
     $this->web_user = $this->drupalCreateUser(array('access field_test content', 'administer field_test content', 'administer content types'));

same

Log in or register to post comments

Comment #8

kid_icarus CreditAttribution: kid_icarus commented 8 August 2012 at 17:29

+++ b/core/modules/field/lib/Drupal/field/Tests/CrudTest.php
@@ -11,6 +11,20 @@ use Drupal\field\FieldException;
+   * @todo: Test creation with:
+   *   - A full fledged $field structure, check that all the values are there.
+   *   - A minimal $field structure, check all default values are set.

Should this perhaps go inside the docblock for testCreateField()?

+++ b/core/modules/field/lib/Drupal/field/Tests/CrudTest.php
@@ -11,6 +11,20 @@ use Drupal\field\FieldException;
+   * @todo: Defer actual $field comparison to a helper function, used for the
+   *   two cases below.

I'm still a little confused by this @todo, I know I changed the wording from 'cases above' to 'cases below', but are the 'two cases' in referense to parent::setUp('field_test', 'number')?

Log in or register to post comments

Comment #9

kid_icarus CreditAttribution: kid_icarus commented 8 August 2012 at 17:30

This comment should be moved up and fixed for the $modules docblock.

Were you talking about adding // field_update_field() tests use number.module to the $modules docblock?

Log in or register to post comments

Comment #10

kid_icarus CreditAttribution: kid_icarus commented 8 August 2012 at 17:47

File	Size
interdiff.txt	1.29 KB
field-1711316-10.patch	17.89 KB

Okay, leaving the @todos per IRC message from @tim.plunkett

Here's a new patch which addresses the blank lines in #8

Log in or register to post comments

Comment #11

kid_icarus CreditAttribution: kid_icarus commented 8 August 2012 at 17:47

Status:

Needs work

» Needs review

Log in or register to post comments

Comment #12

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 8 August 2012 at 17:56

Status:

Needs review

» Needs work

Sorry, I meant leave them where they are in the original code, they're not related to this change.

Log in or register to post comments

Comment #13

kid_icarus CreditAttribution: kid_icarus commented 8 August 2012 at 23:44

Status:

Needs work

» Needs review

File	Size
interdiff.txt	2.28 KB
field-171131-13.patch	17.47 KB

@tim.plunkett No problem :)

Addresses #12

Log in or register to post comments

Comment #14

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 9 August 2012 at 00:52

Status:

Needs review

» Reviewed & tested by the community

Thanks!

Log in or register to post comments

Comment #15

Dries CreditAttribution: Dries commented 9 August 2012 at 20:48

By the way, it wouldn't hurt to re-roll all these issues into one patch. It's not very convenient to have 30 small patches. Any takers to do that? It would be tremendously helpful.