comment_render() iirc. Check it shows up properly, body, subject etc.

Comments

lyricnz’s picture

Status: Active » Needs review
StatusFileSize
new1.22 KB

See attached patch, which uses the reply-to-comment screen to check the output of an individual comment.

boombatower’s picture

Status: Needs review » Needs work

Very simple patch. :)

Comment test results:

473 passes, 0 fails, 0 exceptions

I think instead the test should be placed in postComment() since that already checks for the subject showing up. could just add body and w/e else.

    // Get comment.
    if ($contact !== TRUE) { // If true then attempting to find error message.
      $this->assertText($subject, 'Comment posted.');
      $this->assertTrue((!empty($match) && !empty($match[1])), t('Comment id found.'));
    }

Right after $subject.

lyricnz’s picture

StatusFileSize
new739 bytes

Basically a one-line patch? :) Attached.

lyricnz’s picture

Status: Needs work » Needs review

Tested locally. "479 passes, 0 fails, 0 exceptions" (comment.test)

chx’s picture

Status: Needs review » Needs work

Nope. We want a drupalGet("node/$nid/$cid") and check whether the comment appeared there.

lyricnz’s picture

Status: Needs work » Needs review
StatusFileSize
new1.83 KB

chx is right, while these tests are good, none of them address the issue described in the subject. The attached patch:
- checks comment subject/text on the node/NID/CID page (single comment view - what this issue was asking for)
- checks comment subject/text on the comment/reply/NID/CID page (comment reply view)
- checks comment text on every post-comment page (subject was already checked)

lyricnz’s picture

484 passes, 0 fails, 0 exceptions (comment.test)

chx’s picture

Status: Needs review » Reviewed & tested by the community

/me likes.

catch’s picture

I never ever knew we had node/nid/cid, ever.

dries’s picture

I wonder whether we still generate node/nid/cid links ... If not, this might actually be dead code?

catch’s picture

I don't think there's a link to them anywhere, or if there is, then I'm completely unaware of where that might be. However I checked on a D5 site and those pages do indeed exist. They seem a bit pointless though.

chx’s picture

Haha we have written a test and it revealed that the code being tested is dead?? Because indeed I grepped node/ in comment directory and can't see where this is being created...

lyricnz’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

I did a similar search - for 'node/' and nid and cid, across the entire codebase (since the menu entry that handles this is actually added by node.module), and didn't find anything.

Interestingly, support for this URL seems to be almost a side effect:

function node_menu() {
  // ...
  $items['node/%node'] = array(
    // ...
    'page callback' => 'node_page_view',
    'page arguments' => array(1),
    // ...
    'type' => MENU_CALLBACK);
  // ...
}

function node_page_view($node, $cid = NULL) {
  drupal_set_title(check_plain($node->title));
  return node_show($node, $cid);
}

function node_show($node, $cid, $message = FALSE) {
  // ...
  $output = node_view($node, FALSE, TRUE);

  if (function_exists('comment_render') && $node->comment) {
    $output .= comment_render($node, $cid);
  }
  // ...
  return $output;
}

So removing it would (a) change the functions parameters above, and (b) not save much code. In any case, removing it seems like a seperate issue - this issue is about a test-case: as long as the code is present, the test is valid, right? The testcase will (correctly) trip up if someone removes the code, and can be fixed then.

lyricnz’s picture

Oops, didn't mean to change status

lyricnz’s picture

Status: Patch (to be ported) » Reviewed & tested by the community
StatusFileSize
new1.84 KB

Re-roll, post #302396: Make tests more granular. 484 passes, 0 fails, 0 exceptions.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yeah, I agree that while this code exists, we should probably write tests for it, and the question of whether or not it's truly dead (I could see the use case for "permalinking" to a comment, maybe...) taken up in a separate issue. lyricnz's research into it in #13 makes it a bit muddy as to whether we'd actually be saving anything or not.

Test looks good. Committed to HEAD. Thanks!

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.