Contact list is empty.

You are correct, looks like the 's' was accidentally dropped in #302219: DBTNG: contact.

Reviewed the patch and it looks good, applied the patch and it works as advertised.

Oops. :) Committed, thanks.

I thought about modifying the contact tests to catch this, but I'm not exactly sure how to do that given that the name shows up as part of the drupal_set_message().

@webchick: probably wise to add/change a test for this. The issue isn't the "category added message", which is taken care of already:

$this->assertRaw(t('Category %category has been added.', array('%category' => $category)), t('Category successfully added.'));

The problem is the categories table in the body of admin/build/contact. Tests should probably be changed to include assertions that the new category is appearing in the table. I think it's just a matter of adding one extra assertion to the existing tests.

Should this issue be re-opened for the test or should a new issue be created?

We can re-open this. I thought of changing that to just $this->assertText($category, t('Found the category')); but I'm not sure that would have failed, since it also appears in the drupal_set_message(). If we had to break into doing $this->assertXPath() or similar to make sure it was enclosed in table tags, that seems like it'd be pretty crazy and inconsistent with what we do elsewhere.

Well, we could either:

1. add a couple of categories, then reload the page (to get rid of the messages), then verify that the categories are still listed on the page;
2. or use the brand new assertNoUniqueText added in #402804: New Assertion: Check for test being found only once to make sure the category name appears more than once.

Both would be simpler than the XPath route.

@pp: since you wrote the initial patch, would you be willing to submit a new patch containing changes to the tests for this scenario?

Just moving the status to review for the patch submitted by pp.

Hi pp, thanks for rolling this patch!

Some comments after reviewing the added test:

1. The original test adds more than one category (and verifies that the "Category was created" message is displayed for each one). It probably makes sense to do the same for the category table check.
2. Coding Standards dictate that comments should end with a period:
   

   +    // Test categories table
   

   I also think it should be a bit more verbose (something like "Make sure the newly created category is included in the list of categories."?)

3. The assertion message should tell the user what the expected outcome is (or what you are verifying). So instead of "Category list not shown" it should read something like "New category included in categories list.".

   +    $this->assertNoUniqueText($category, t('Category list not shown'));
   

I haven't applied the patch yet, but other than the comments above it looks good. Can you re-roll this patch with these changes?

Hmm.. this passed before. Wonder why it failed this time. =\

@JamesAn: Testing bot weirdness. :)

Looks good and I approve.

Committed to CVS HEAD. Thanks!