GHOP #157: Create simpletests for CCK field storage

The official task associated with this issue can be found at: http://code.google.com/p/google-highly-open-participation-drupal/issues/...

How mature is drupal test case's drupalCreateContentType function? I'm using it three times in a test function, and getting alot of simpletest exceptions when later, node_delete($nid); and node_type_delete($type); are called in the tear down.

So as a minimal test:

   function testExample() {
    $type1 = $this->drupalCreateContentType();
    $node1 = $this->drupalCreateNode(array('type' => $type1->name));
  } 

Causes 1 pass and 22 expections:

status: <em>simpletest_oEuDeMlu</em> has been deleted. at [E:\HTML\Apache\users\drupal60_head\modules\simpletest\drupal_test_case.php line 488]	OK
Unexpected PHP error [Undefined index: created] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\simpletest\drupal_test_case.php line 56]	EXCEPTION
Unexpected PHP error [Invalid argument supplied for foreach()] severity [E_WARNING] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content.module line 1037]	EXCEPTION
Unexpected PHP error [Invalid argument supplied for foreach()] severity [E_WARNING] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content.module line 1077]	EXCEPTION
Unexpected PHP error [Invalid argument supplied for foreach()] severity [E_WARNING] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content.module line 1037]	EXCEPTION
Unexpected PHP error [Invalid argument supplied for foreach()] severity [E_WARNING] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content.module line 953]	EXCEPTION
Unexpected PHP error [Invalid argument supplied for foreach()] severity [E_WARNING] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content.module line 990]	EXCEPTION
Unexpected PHP error [Invalid argument supplied for foreach()] severity [E_WARNING] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content.module line 920]	EXCEPTION
Unexpected PHP error [Invalid argument supplied for foreach()] severity [E_WARNING] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content.module line 1037]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content_crud.inc line 588]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content_crud.inc line 592]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\cck\fieldgroup.module line 495]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\cck\fieldgroup.module line 496]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\taxonomy\taxonomy.module line 726]	EXCEPTION

I'd rather use

$node1 = $this->drupalCreateNode(array('type' => $type1->type));

than

$node1 = $this->drupalCreateNode(array('type' => $type1->name));

but I don't think this is the cause.

- the first warning looks like a bug in drupalCreateNode() : refers to $default['created'], but the 'created' key isn't initialized (only 'changed'...)
- for the other warnings : could you make sure you use latest CCK HEAD ? The line numbers do not seem to match the errors mentioned.

These should match up: (changing name to type has no effect on these)

status: <em>simpletest_AqQJbKvT</em> has been deleted. at [E:\HTML\Apache\users\drupal60_head\modules\simpletest\drupal_test_case.php line 488]	OK
Unexpected PHP error [Undefined index: created] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\simpletest\drupal_test_case.php line 56]	EXCEPTION
Unexpected PHP error [Invalid argument supplied for foreach()] severity [E_WARNING] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content.module line 1027]	EXCEPTION
Unexpected PHP error [Invalid argument supplied for foreach()] severity [E_WARNING] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content.module line 1067]	EXCEPTION
Unexpected PHP error [Invalid argument supplied for foreach()] severity [E_WARNING] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content.module line 1027]	EXCEPTION
Unexpected PHP error [Invalid argument supplied for foreach()] severity [E_WARNING] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content.module line 943]	EXCEPTION
Unexpected PHP error [Invalid argument supplied for foreach()] severity [E_WARNING] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content.module line 980]	EXCEPTION
Unexpected PHP error [Invalid argument supplied for foreach()] severity [E_WARNING] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content.module line 910]	EXCEPTION
Unexpected PHP error [Invalid argument supplied for foreach()] severity [E_WARNING] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content.module line 1027]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content_crud.inc line 588]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content_crud.inc line 592]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\cck\fieldgroup.module line 495]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\cck\fieldgroup.module line 496]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\taxonomy\taxonomy.module line 726]	EXCEPTION

If not, the lines in question from content.module / crud.inc are;

 foreach ($type['fields'] as $field) { // 1027
foreach ($type['fields'] as $field) { // 1067
foreach ($type['tables'] as $table) { // 943
foreach ($type['fields'] as $field) { // 980
foreach ($type['tables'] as $table) { // 910

$fields = content_field_instance_read(array('type_name' => $info->type)); // 588
$table = _content_tablename($info->type, CONTENT_DB_STORAGE_PER_CONTENT_TYPE); // 592 

OK, thanks for the update.

Several things happen here :
- drupalCreateContentType() needs to clear core's cached type info after saving the new content type.
I'll roll a patch for drupal_test_case.php, for now you can manually change the code in drupalCreateContentType() to :

function drupalCreateContentType($settings = array()) {
  (...)
  
  node_type_save($type);
  node_types_rebuild(); // <--

  $this->_cleanupContentTypes[] = $type->type;
  return $type;
}

- CCK's own cached info about content types needs to be cleared as well.
Currently, content.module reacts to node_type_save by *flagging* a rebuild, that gets executed on next page request (this was to done to save multiple rebuilds in one request). This is obviously not enough for us here. Not sure yet how we want to fix this, for now you need to call content_clear_type_cache() after creating your content types :

function testExample() {
  $type1 = $this->drupalCreateContentType();
  content_clear_type_cache();
  $node1 = $this->drupalCreateNode(array('type' => $type1->type));
}

Works for me, no errors.

[edit : $type1->type, not $type1->name - wrong copy/paste]

Another bug in drupalCreateContentType:
$name = $this->randomName(3, 'type_');
Should be:
$name = strtolower($this->randomName(3, 'type_'));
As the machine-readable name for content types should contain only lowercase, numbers and underscores (atleast according to the content type creation form in admin), and/or node_type_save should refuse names with uppercase.

Written first draft of the test 1.

I'm getting a strange behaviour of drupalCreateContentType when used within two tests;

  function testA() {
    $type = $this->drupalCreateContentType();
  }
  
  function testB() {
    $type = $this->drupalCreateContentType();
  }

Generates 14 exceptions:

Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content_crud.inc line 588]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\cck\content_crud.inc line 592]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\cck\fieldgroup.module line 495]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\cck\fieldgroup.module line 496]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\comment\comment.module line 264]	EXCEPTION
Unexpected PHP error [Trying to get property of non-object] severity [E_NOTICE] in [E:\HTML\Apache\users\drupal60_head\modules\taxonomy\taxonomy.module line 726]	EXCEPTION

Commenting out the creation line from either test causes the exceptions to disappear.

Implemented 2nd test and made a number of refactorings.

Woo, impressive job !
Still wrapping my head on the node data part, here's my comments so far :

- The content_simpletest() hook is not needed anymore with latest simpletest.module. Thus, you can simply upload your content_crud.test file in the thread without rolling a patch

- The set of helper functions would probably deserve a ContentCrudTest superclass on their own, with the actual tests deriving from it.

- Some functions are only internal helpers, not intended to be actually used in tests, right ? If so, they might be worth prefixing with a '_', just to make the 'test API' clearer.

- createFieldText() : I'd rather have text_processing explicitly set than made a default.
Also, that function should probably make sure text.module is enabled, but I'm really nitpicking :-).

- createRandomTextFieldData(), getContentTypeTableName(), getFieldTableName() function definitions should be moved up next to the group of schema related assert functions.

- assert messages should be unified : 'Table x should not exist' does not match 'Node data y is correct' and 'Table z matches schema' pattern

- In the tests themselves, we should probably avoid the random type and field name generation - too complicated to track. Sticking to fields and types with definite names ('st_fN', 'st_tN') would be easier.

- Not sure assertSchemaMatchesField() should automatically add 'delta'. Let it be explicitely expected in the $extra_columns (simply don't auto-prefix that one)

- Do you think the assertTableNotExists() used in the actual tests could be handled by assertSchemaMatchesContentType / assertSchemaMatchesField ?
More generally, I've been thinking about simplifying schema checking functions and use a single
assertSchemaMatchesTables() function, with an exhaustive array as parameter :

$tables = array(
  'per_field' => array(
    'st_f1' => array('delta', 'field_f1' => array('value, 'format'),
    'st_f2' => NULL,    // no content_field_f2 table
  ),
  'per_type' => array(
    'st_t1' => array('field_f2' => array('value'), 'field_f3' => array('value', 'format')), 
    'st_t2' => array(), // only 'nid' and 'vid' columns
    'st_t3' => NULL,    // no content_type_t3 table
  ),
);
$this->assertSchemaMatchesTables($tables);

Requires a little more writing in the tests themselves, but makes it crystal clear what we expect on each step, while not needing to figure out what assert function should be called. What do you think ?

- Getting fails for 'Table content_type_type_* should not exist' : ok, that's bug http://drupal.org/node/201329 (tables not cleaned up on field deletion)

- Getting a fail for 'Node data [field_*][0][value] is correct' on line 352. This means a bug in CCK or in the test :-) Do you get that as well ?

- It seems I'm not getting any of the exceptions you mention in #8 ?

OK, I spent some more time on the node checks part.
Not completely simple. The dangerr here is to over-engineer this and force us to be sure our test framework is bug-free itself :-)
This being said, the main goal of this task is to provide the right tools for writing tests, so finding the right balance is not easy.

A few general remarks :
- More code comments would be really welcome at places. A function like _compareArrayForChanges for instance is a little obscure.
- This includes PHPDoc for the functions : what are the arguments, what's their type (a node, a node id, a type, a type name...)
- More generally, variables and parameters names are a bit confusing : sometimes $node is a node, sometimes it's an index... It's important that variable names reflect what they contain (type object, type index in $this, node, field, field name, etc...), more than what we use them for ($share_with...), which should be told by code comments. This is true for a large part of the current code, and this would make the test API easier to grasp and the tests themselves easier to read and check.

More specific to the node tests code :
- acquireNodes() : I'd think $count should be the number of nodes created for each content type (not the *total* number of nodes)
- assertNodeStillUnchanged() : could have a simpler name ?
- Not sure we want the shorthands allowed by parts like :

function assertNodeStillUnchanged($nid = NULL, $changes = NULL) {
  if (!isset($nid)) {
    $nid = $this->previous_node_changes_nid;
  }
  if (!isset($changes)) {
    $changes = $this->previous_node_changes;
  }

Shorthands make tests easier to write, but more complicated to follow.

- Similarly to my remark on schema comparison in my previous post : I think it would be clearer if we explicitely wrote what the node should be and tested it using one single test function, that takes care of checking everything that should is here and nothing's here that shouldn't (only looking at $node->field_* keys, of course - we could probably use a helper function that returns the field_* namespace part of a node).

$values = array(
  'field_st_f1' => array(array('value' => 'foo', 'format' => 0), array('value' => 'bar', 'format' => 0)),
  'field_st_f2' => NULL // no field_st_f2 should be found.
);
$this->saveNodeValues($node, $values); // Merges the new values in the node and saves it.
$this->assertNodeValues($node->nid, $values); // Loads the node and compares.

Your experience on writing the tests makes your advice precious here, of course :-)

I suggest we primarily focus on the db schema checks right now. At worse, if we can get this straight and clean and committed, then we have gained something really valuable, and node data chacks can be tackled later.

I'm currently acting upon his comments, and should have a new version in a few hours.

- The content_simpletest() hook is not needed anymore with latest simpletest.module. Thus, you can simply upload your content_crud.test file in the thread without rolling a patch
Attached as tar.gz, as .test files aren't allowed :[

- The set of helper functions would probably deserve a ContentCrudTest superclass on their own, with the actual tests deriving from it.
Done

- Some functions are only internal helpers, not intended to be actually used in tests, right ? If so, they might be worth prefixing with a '_', just to make the 'test API' clearer.
Prefixed the functions not intended to be called from outside, and pruned off some unused functions.

- createFieldText() : I'd rather have text_processing explicitly set than made a default.
Also, that function should probably make sure text.module is enabled, but I'm really nitpicking :-).
Done

- createRandomTextFieldData(), getContentTypeTableName(), getFieldTableName() function definitions should be moved up next to the group of schema related assert functions.
Moved them to correct places, and started each group of functions with a short comment saying what group it is

- assert messages should be unified : 'Table x should not exist' does not match 'Node data y is correct' and 'Table z matches schema' pattern
Changed the message for tables being absent. I didn't change any others, as I assume you were looking at the "not" in the message, which nothing else has.

- In the tests themselves, we should probably avoid the random type and field name generation - too complicated to track. Sticking to fields and types with definite names ('st_fN', 'st_tN') would be easier.
Given types the name simpletest_tN and fields the name simpletest_fN

- Not sure assertSchemaMatchesField() should automatically add 'delta'. Let it be explicitely expected in the $extra_columns (simply don't auto-prefix that one)
- Do you think the assertTableNotExists() used in the actual tests could be handled by assertSchemaMatchesContentType / assertSchemaMatchesField ? ...
Changed to using a assertSchemaMatchesTables function for all table assertions.

- Getting a fail for 'Node data [field_*][0][value] is correct' on line 352. This means a bug in CCK or in the test :-) Do you get that as well ?
I get it, but not sure of the cause at the moment.

- It seems I'm not getting any of the exceptions you mention in #8 ?
I'm not either any more, which is perplexing, but convenient.

- More code comments would be really welcome at places. A function like _compareArrayForChanges for instance is a little obscure.
- This includes PHPDoc for the functions : what are the arguments, what's their type (a node, a node id, a type, a type name...)
Given all the functions in the test case class a docblock

- More generally, variables and parameters names are a bit confusing...
Tried to standardise functions so that they can take either an entity, or the index of an acquired entity, with a argument named $node or $content_type. Changed some variable names to include what they contain, but there are probably some more that can be changed.

- acquireNodes() : I'd think $count should be the number of nodes created for each content type (not the *total* number of nodes)
Done.

- assertNodeStillUnchanged() : could have a simpler name ?
Now called assertNodeValues, with the partner function called assertNodeSaveValues

- Not sure we want the shorthands allowed by parts like :
function assertNodeStillUnchanged($nid = NULL, $changes = NULL) {
The function now requires both parameters. The *field functions still remember the last used field and use it as default, but I think for fields it is clearer as we only have one field?

- Similarly to my remark on schema comparison in my previous post : I think it would be clearer if we explicitely wrote what the node should be and tested it using one single test function...
Haven't done this yet, as the DB schema checks were deemed more important.

(Landed in Chicago, finally got a chance to check my mail)
Thanks for the update. Completely agreed on RTBC.

I'd still be interested in your opinion on the shaping on node tests assert API, so that we can possible refine, but there's no reason it should hold the current patch, which more than fulfills the task at hand here .

Thanks for the great job, Corsix ! I assume you don't need to be reminded to attach the latest code to the google tracker for the task :-)

CCK gets simpletests ! I wouldn't be surprised to learn that the Ewoks are dancing right now...

Committed to HEAD. Great job.