node.test cleanup

A prefix as in:

$edit['title'] = "title_" . $this->randomName(4);

Isn't even needed or preferred it would be better to just do

$edit['title'] = $this->randomName(8);

I will try and take a look at this in more detailed and make changes as I see fit later in the week.

While were are in cleanup mode I've made another issue: #295864: SimpleTest: Change randomName() method to randomString()

Working on this.

Took care are a ton of formatting, new API refactoring, poor comments, dumb assertions, etc.

The tests still use page content type, but that can be the next change, if necessary. The tests, as are they sit, are much more worthy to be used as an example.

All node test still pass.

Patch requires #299186: SimpleTest: assertFieldByXPath does not work for selects or textareas with value to work.

Still applies.

No long applies.

patching file modules/node/node.test
Hunk #5 FAILED at 390.
1 out of 5 hunks FAILED -- saving rejects to file modules/node/node.test.rej

Wish these patches wouldn't be overlooked so long. Been ready since August 23.

I'll work on re-roll.

Ok..I've re-rolled patch.

Note it still requires: #299186: SimpleTest: assertFieldByXPath does not work for selects or textareas with value.

Ran test suite...all passes!

With #299186: SimpleTest: assertFieldByXPath does not work for selects or textareas with value All tests pass.

5589 passes, 0 fails, 0 exceptions

I didn't spend alot of time looking at the before, but this definitely cleans up a few things.

A few points, in callNodeTeaser() $expectedTeaser, should probably just be $expected, as in testFirstSentenceQuestion().

Also Line 352, 'consistency,' should probably just be 'presence.'

Other than that, looks good.

Figured I might as well roll a patch while I'm at it.

Essentially the same as my previous patch and it has had two sets of eyes look at it.

+      'description' => t('Creates a node, then a user tries various revision
                           actions such as viewing, reverting to, and deleting revisions.'),

I realize this wasn't touched directly by your patch, but that should be done on one line.

-   * Simpletest test. A real-life example of the above edge case.
+   * Test teaser with long example.

+1 for getting rid of that UberBabe stuff, but I'm not sure if the Lorem Ipsum replacement handles the actual edge case that was described. Any idea what that that edge case in fact is? I'd like to see it better documented in the testFirstSentenceQuestion() test above.

+    $this->assertLink(t('Edit'), 0, t('Edit task found.'));

Probably edit "tab" found?

-    $this->assertNotNull($node, 'Node found in database');
+    $this->assertTrue($node, t('Node found in database.'));

That doesn't really make sense. Nodes that are found in the database aren't booleans. In fact, if this test is passing, it probably means we need to fix assertTrue().

+    $test_user = $this->drupalCreateUser(array('create page content', 'edit own page content'));
+    $this->drupalLogin($test_user);

Everywhere else it seems this is called $web_user. Let's be consistent.

+    // Create user with permission to edit node.
+    $this->drupalLogin($test_user);

Comment does not match code.

Actually running assertTrue on a $node makes perfect sense. As assertTrue casts to boolean whatever is passed to it, the only case it can fail if that node_load returns FALSE and the only way that can happen is if the node is not in the DB. In this case, think of assertTrue as "assertNotFalse".

Now with 30% more screenshots!!

Reworked most of the descriptions, see screenshot below.

'Edit task..' => 'Edit tab...'

$test_user => $web_user

Re: out of place comment, in code, that reads

    // Create user with permission to edit node.
    $web_user = $this->drupalCreateUser(array('administer nodes'));
    $this->drupalLogin($web_user);

No idea about the above edge case for node_teaser().

Just realized that Node preview test doesn't actually create a node, edited description, see screenshot.

Had

$$web_user

I removed extra $.

I diffed that patches...changes look good.

Yeah, except:

    * Tests an edge case where if the first sentence is a question and
    * subsequent sentences are not.

That comment still needs clarifying. CVS annotate should do the trick.

Also, I saw this:

+    $this->drupalPost(NULL, $edit, t('Save'));

NULL? I don't find that more legible than - $this->drupalPost("node/$node->nid/edit", $edit, t('Save'));

Reason for NULL being that we assure that the link clicked work, also 1 less page request, and tests internal framework. As that is the actual flow.

click link -> type values -> submit page
not:
click link -> reload page -> type values -> submit page.

Found original issue and added to comment.

Excellent. Thanks, committed.

I also added a little comment above that drupalPost line.

After this has been applied, I get two failed tests:

Node edit:
24 passes, 1 fail, 0 exceptions
Body field displayed. Browser node.test 288 PageEditTestCase->testPageEdit()

Node preview:
18 passes, 1 fail, 0 exceptions
Body field displayed. Browser node.test 341 PagePreviewTestCase->testPagePreview()

So far I haven't investigated further why they fail.

@c960657: did you apply only this patch? If so that is the problem. This patch requires another, and both have been committed. Do you still get issue when you update to HEAD?

I still get failed tests with HEAD and a clean database.

It looks as if there is a problem with DrupalWebTestCase::assertFieldByXPath(). It does find the textarea element, but isset($field[0]) returns false.

I am no SimpleXML expert (actually I don't like SimpleXML, because there is far to much magic going on for my taste), but making the following change fixes the problem:

--- modules/simpletest/drupal_web_test_case.php 17 Sep 2008 07:11:58 -0000      1.41
+++ modules/simpletest/drupal_web_test_case.php 17 Sep 2008 21:06:43 -0000
@@ -1459,13 +1459,13 @@ class DrupalWebTestCase {
               $items = $this->getAllOptions($field);
               if (!empty($items) && $items[0]['value'] == $value) {
                 $found = TRUE;
               }
             }
           }
-          else if (isset($field[0]) && $field[0] == $value) {
+          else if ($field == $value) {
             // Text area with correct text.
             $found = TRUE;
           }
         }
       }
     }

I am using PHP 5.2.0-8+etch11 (Debian) with libxml 2.6.27.

I'll look into when I get home.

hmmm both mine pass. open separate issue if this persists.

The assertFieldByXPath() problem is now filed as #310113: SimpleTest: assertFieldByXPath() doesn't work in some environments.