Problem/Motivation

There are seven separate assertions (or 118 tests) which currently assume that all node teasers have a "Read more" link. This works only because of a bug,

See #823380: Read More link is always visible on teaser.

In order to reinstate the D6 behavior that a "Read more" link only appears when there is "more" to read, the conflicting tests must be rewritten or removed.

The conflicting tests were added in the following issues:

This issue is major because it blocks #823380, which is major.

Proposed resolution

The seven assertions should be rewritten to detect node visibility in some other way.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Category: bug » task

Re-classifying as a task, since there's nothing really broken here.

webchick’s picture

Issue tags: +Novice

And in glancing at the test failures in http://qa.drupal.org/pifr/test/179314, this seems like a pretty straight-forward thing to fix. Just remove the bogus assertion from CommentHelperCase->testCommentNewCommentsIndicator().

Tentatively tagging as Novice.

pillarsdotnet’s picture

pillarsdotnet’s picture

Status: Active » Needs review
FileSize
3.23 KB

Tests commented out, but I doubt this will really fly.

Status: Needs review » Needs work

The last submitted patch, remove-read_more-tests-1300568-4.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
4.08 KB

Ripping out a bunch of other tests that have a cascading dependency on the existence of the "Read more" link.

And this is quite definitely not a "Novice" task, despite my "Novice" approach so far.

pillarsdotnet’s picture

Ripped out another one.

pillarsdotnet’s picture

Issue summary: View changes

We now have an accurate account of both the number of assertions (7) and the number of tests (89) involved.

pillarsdotnet’s picture

Issue summary: View changes

Correct the total number of tests as reported by test results attached to http://drupal.org/node/823380#comment-5085976

pillarsdotnet’s picture

Issue summary: View changes

Added links.

pillarsdotnet’s picture

pillarsdotnet’s picture

Status: Needs review » Closed (won't fix)
joachim’s picture

Status: Closed (won't fix) » Needs review

See my comment over there.

joachim’s picture

Status: Needs review » Needs work

An alternative way to fix

+++ b/modules/comment/comment.test
@@ -263,9 +263,10 @@ class CommentHelperCase extends DrupalWebTestCase {
     $this->drupalGet('node');
     $this->assertNoLink(t('@count comments', array('@count' => 0)));
     $this->assertNoLink(t('@count new comments', array('@count' => 0)));
-    $this->assertLink(t('Read more'));
-    $count = $this->xpath('//div[@id=:id]/div[@class=:class]/ul/li', array(':id' => 'node-' . $this->node->nid, ':class' => 'link-wrapper'));
-    $this->assertTrue(count($count) == 1, t('One child found'));
+    // @todo: Come up with a test that does not require a "Read more" link.
+    // $this->assertLink(t('Read more'));
+    // $count = $this->xpath('//div[@id=:id]/div[@class=:class]/ul/li', array(':id' => 'node-' . $this->node->nid, ':class' => 'link-wrapper'));
+    // $this->assertTrue(count($count) == 1, t('One child found'));

These can just be ripped out. The presence of the Read More link when there are new comments is irrelevant and the 'count comments' links are a good enough test.

+++ b/modules/node/node.test
@@ -1085,17 +1087,23 @@ class NodeAccessBaseTableTestCase extends DrupalWebTestCase {
+       * @todo: Find a way to test for the presence of a node that does not
+       * require the presence of a "Read more" link.
       foreach ($this->xpath("//a[text()='Read more']") as $link) {
         $this->assertTrue(preg_match('|node/(\d+)$|', (string) $link['href'], $matches), 'Read more points to a node');
         $this->nids_visible[$matches[1]] = TRUE;
       }
+      */
       foreach ($this->nodesByUser as $uid => $data) {

Here we're going to need a replacement.

Some thoughts that spring to mind:

- ensure that read more *is* visible by setting a short teaser length in node tests
- test for the node titles

xjm’s picture

Assigned: Unassigned » xjm

Since the node titles should link to the nodes and also would (presumably) not do so if access wasn't allowed, that seems like the correct fix to me. I'll work on this.

xjm’s picture

Status: Needs work » Needs review
FileSize
2.08 KB

So, looking at this, I'm not sure this actually should be a separate issue from #823380: Read More link is always visible on teaser.. It could be argued that core currently has an assumption that teaser <=> read more link, and if we are changing that assumption in the other issue, then the other issue would presumably update everything needed to change that assumption, including the tests.

But, that issue is a monster, and this one isn't, so here's a patch.

Tor Arne Thune’s picture

Are there other tests in core that makes sure the read more links work?

xjm’s picture

For reference, the assertTaxonomyPage() method is used only in testNodeAccessBasic().

xjm’s picture

Re #14: I guess that's another reason this probably should be in the other issue, because there is no specific "Test read more" coverage that I found, so we would want to add new tests to that patch instead. Edit: actually there already is test coverage for it being added in #823380-199: Read More link is always visible on teaser..

Tor Arne Thune’s picture

Yeah, that makes sense. This issue can probably be closed and the tests incorporated into a patch for the other issue, as suggested by xjm.

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs review » Closed (duplicate)

Closing as duplicate. I'll incorporate this patch into that issue.

xjm’s picture

Issue summary: View changes

Point links to patches.