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.
Comment | File | Size | Author |
---|---|---|---|
#13 | read-more-1300568.patch | 2.08 KB | xjm |
#7 | remove-read_more-tests-1300568-7.patch | 4.44 KB | pillarsdotnet |
#6 | remove-read_more-tests-1300568-6.patch | 4.08 KB | pillarsdotnet |
#4 | remove-read_more-tests-1300568-4.patch | 3.23 KB | pillarsdotnet |
Comments
Comment #1
webchickRe-classifying as a task, since there's nothing really broken here.
Comment #2
webchickAnd 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.
Comment #3
pillarsdotnet CreditAttribution: pillarsdotnet commented@webchick: See http://qa.drupal.org/pifr/test/179334
Comment #4
pillarsdotnet CreditAttribution: pillarsdotnet commentedTests commented out, but I doubt this will really fly.
Comment #6
pillarsdotnet CreditAttribution: pillarsdotnet commentedRipping 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.
Comment #7
pillarsdotnet CreditAttribution: pillarsdotnet commentedRipped out another one.
Comment #7.0
pillarsdotnet CreditAttribution: pillarsdotnet commentedWe now have an accurate account of both the number of assertions (7) and the number of tests (89) involved.
Comment #7.1
pillarsdotnet CreditAttribution: pillarsdotnet commentedCorrect the total number of tests as reported by test results attached to http://drupal.org/node/823380#comment-5085976
Comment #7.2
pillarsdotnet CreditAttribution: pillarsdotnet commentedAdded links.
Comment #8
pillarsdotnet CreditAttribution: pillarsdotnet commentedThe conflicting tests were added by the following issues:
#983632-41: New comment marker does not work
#1204658: Always use query metadata to specify the node access base table
Comment #9
pillarsdotnet CreditAttribution: pillarsdotnet commentedClosing per #823380-202: Read More link is always visible on teaser.
Comment #10
joachim CreditAttribution: joachim commentedSee my comment over there.
Comment #11
joachim CreditAttribution: joachim commentedAn alternative way to fix
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.
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
Comment #12
xjmSince 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.
Comment #13
xjmSo, 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.
Comment #14
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedAre there other tests in core that makes sure the read more links work?
Comment #15
xjmFor reference, the assertTaxonomyPage() method is used only in testNodeAccessBasic().
Comment #16
xjmRe #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..
Comment #17
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedYeah, that makes sense. This issue can probably be closed and the tests incorporated into a patch for the other issue, as suggested by xjm.
Comment #18
xjmClosing as duplicate. I'll incorporate this patch into that issue.
Comment #18.0
xjmPoint links to patches.