We managed to break this from #557292: TF #3: Convert node title to fields. We should make sure this path is test-covered, and/or if it is, figure out why the tests didn't catch this (because garland overrides node.tpl.php?)

CommentFileSizeAuthor
#14 node-test-601308-14.patch1.65 KBpcarman
#8 node-601308-8.patch898 bytessfyn
#2 node-601308-2.patch872 bytessfyn

Comments

sfyn’s picture

sfyn commented

Is this path test-covered?

Yes - NodeTitleTestCase tests this at line 1251 of node.test

However it fails if the markup surrounding node title is changed in anyway form an anchor inside of an h2 tag. This means it could potentially fail on any number of themes overriding node.tpl.php.

Which brings us to garland - because themes overide system default node.tpl.php, it is absolutely possible for simpletest to not test the contents of the default system template.

I am looking at rewriting the test to be more responsive to changes in theme templates.

sfyn @D7csmtl

sfyn’s picture

sfyn commented
StatusFileSize
new node-601308-2.patch872 bytes

@D7csmtl

For starters, lets add a pass to check for clickable links via /node

sfyn’s picture

sfyn commented

@D7csmtl

Fun fact - this test passes at times when the previous pass (testing for a title in an <a&rt; within an <h2&rt;) fails.

sfyn’s picture

sfyn commented

@D7csmtl

These tests were originally introduced in http://drupal.org/node/557292#comment-2134726

sfyn’s picture

sfyn commented

@D7csmtl

Maybe the best way of getting around this problem and ensuring we test the default system .tpl.phps instead of garlands is to have simpletest use stark as its testing theme.

Therefore, new issue: Simpletest does not test system default templates (like node.tpl.php)

sfyn’s picture

sfyn commented
Status: Active » Needs review

go testbot go

Status: Needs review » Needs work

The last submitted patch, node-601308-2.patch, failed testing.

sfyn’s picture

sfyn commented
Status: Needs work » Needs review
StatusFileSize
new node-601308-8.patch898 bytes

reroll

sfyn’s picture

sfyn commented
Issue tags: +D7csmtl
pcarman’s picture

pcarman commented
Status: Needs review » Reviewed & tested by the community

@drupalcon

Test code has been reviewed and all test pass. Looks like the changes to the test fulfill the requirement.

By the way this is my first issue reviewed. Let me know if you have suggestions.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Ahhh first patch review. :) That means I get to be extra picky! :D Thanks for helping out!

Actually the patch looks pretty fine, and for the most part these are all minor comments, related to the coding standards.

+++ modules/node/node.test	2010-02-19 14:55:26.000000000 -0500
@@ -1233,6 +1233,7 @@ class NodeTitleTestCase extends DrupalWe
+      'promote' => 1, //Add the node to the frontpage so we can test if teaser links are clickable

Let's move those comments above the $settings = array( ... ) line, since coding standards dictate that we don't use comments to the side. Also, they should begin with a space between the // and the first letter (like the "Create "Basic page" content..." one does above) and should end in a period.

+++ modules/node/node.test	2010-02-19 14:55:26.000000000 -0500
@@ -1249,7 +1250,12 @@ class NodeTitleTestCase extends DrupalWe
+    $this->drupalGet("node");

"node" should be in single quotes, since there's no variable in it.

+++ modules/node/node.test	2010-02-19 14:55:26.000000000 -0500
@@ -1249,7 +1250,12 @@ class NodeTitleTestCase extends DrupalWe
+    $this->clickLink($node->title);

I'm not sure if there is a security problem with this line. Probably not, but check other clickLink() calls in other tests and just make sure that we don't have to wrap that in check_plain() first.

+++ modules/node/node.test	2010-02-19 14:55:26.000000000 -0500
@@ -1249,7 +1250,12 @@ class NodeTitleTestCase extends DrupalWe
+  ¶

Trailing whitespace is introduced.

112 critical left. Go review some!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work
gowriabhaya’s picture

gowriabhaya commented
Issue tags: +TestingPartySF

Code sprint tag

pcarman’s picture

pcarman commented
Status: Needs work » Needs review
StatusFileSize
new node-test-601308-14.patch1.65 KB

Updated the patch based on webchick's comments. The patch has also been rerolled against HEAD.

fending’s picture

fending commented
Status: Needs review » Reviewed & tested by the community

RTBC #14, which includes most of @webchick suggestions. I don't think check_plain() actually adds any value here as it's done elsewhere; I agree with patch author @pcarman on this one. Works great, less filling.

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
commented

I tested this, too, and did not encounter any errors.

afreeman’s picture

afreeman commented

+1 tested, all tests pass and code style looks good.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks for your contribution! :)

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests, -D7csmtl, -TestingPartySF

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

Issue tags: +Needs tests, +D7csmtl, +TestingPartySF

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.