GHOP #122: Write Simpletest tests for the Forum module

GHOP issue: http://code.google.com/p/google-highly-open-participation-drupal/issues/...

Ok - looks like a good start. However - the 7 tests don't actually have to be separate DrupalTestCase objects - you can have all of this together in one "test" as long as these 7 pieces of functionality are tested. Or group them in 2 or 3 tests as seems most logical.

Also, if you still have repeated code, you could play the trick I used here: http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/simpletest/...

The latter tests extend the first object which defines common method in addition to those already in DrupalTestCase.

ok - looking better.

A couple minor suggestions to start-

I'd rename AddForumTest to something like DrupalForumTestCase.

This should have no "testX" methods, since I think they will get run twice if a later object extends it. It should only have common methods like your function createForum().

Also, the second object should extend DrupalForumTestCase also, this way you can omit:

    // Enable the forum module and its dependencies
    $this->drupalModuleEnable('taxonomy');
    $this->drupalModuleEnable('comment');
    $this->drupalModuleEnable('forum');

since the setup function will run.

Finally - minor code style issue - don't have any white space between the function name and parens in the function definition.

ok- obviously I'm not paying attention. You're right about the tests not running twice. Sorry.

How about merging the first two objects together so there is no get_info() that doesn't run an actual test?

testing this now - it's weird with the menu tests, they only run once, but if I add a trivial additional test it runs multiple times.

  function testFoo() {
    $this->assertFalse(1, 'foo');
  }

oh wait - I see - I used the same function name in both classes in menu_module.test. I'm too clever for myself...

Ok, a minor problem with the tests for the expected text like:

$this->assertWantedRaw('Created new forum container <em>'. $title .'</em>.', t('New forum container has been created'));

This will break if localization is turned on. You need to write it with t() mimicking the exact form from forum module like from modules/forum/forum.admin.inc line 90:

drupal_set_message(t('Created new @type %term.', array('%term' => $form_state['values']['name'], '@type' => $type)));

So your code needs to be like:

$type = t('forum container');
$this->assertWantedRaw(t('Created new @type %term.', array('%term' => $title '@type' => $type)), t('New forum container has been created'));

There are several spots with this problem.

Looking better - all tests pass.

some code style issues still - for example an extra space in array declarations like: array () which should be array()

as before (http://drupal.org/node/201134#comment-669753) try running coder on this.

also, some code could be made more compact, like this:

    // Loop through to find the newly inserted container
    $result = db_query(db_rewrite_sql('SELECT t.tid, t.vid, t.name, t.description, t.weight FROM {term_data} t WHERE t.vid = %d', 't', 'tid'), variable_get('forum_nav_vocabulary', ''));

    $term = array();
    while ($cur_term = db_fetch_array($result)) {
      if ($cur_term['name'] == $title && $cur_term['description'] == $description) {
        $term = $cur_term;
        break;
      }
    }

could be rewritten as something much simpler like:

$term = db_fetch_array(db_query("SELECT * FROM {term_data} t WHERE t.vid = %d' AND t.name = '%s' AND t.description = '%s'", variable_get('forum_nav_vocabulary', ''), $title, $description);

There's probably no reason to use db_rewrite_sql for simpletests.

There are a couple cases of this.

Also - you generally want to write all query strings with double quotes ( " not ' ) since you need to wrap single qoues around %s placeholders.

looks better- though a couple code comments like:

// Loop through to find the newly edited vocabulary

that should be changed since there is no looping. Maybe just:

// Find the newly edited vocabulary

ok - looks very close. A suggestion after reviewing the code again.

This part:

    // Restore the original settings, and use it as an opportunity to test the form again
    $edit = array(
      'name' => $original_settings->name,
      'description' => $original_settings->description,
      'help' => $original_settings->help,
      'weight' => $original_settings->weight
    );

    // Double check that the page says it has edited the vocabulary
    $this->drupalPostRequest('admin/content/taxonomy/edit/vocabulary/'. $vid, $edit, 'Save');
    $this->assertWantedRaw(t('Updated vocabulary %name.', array('%name' => $original_settings->name)), t('Vocabulary has been edited'));
 

could probably be simplified to a single API call like:

$original_settings = (array) $original_settings;
taxonomy_save_vocabulary($original_settings);

rather than re-testing the thing we tested before. It doesn't matter much, but as it is now will give a redundant failure message.

I don't see any other problems - good job.