Posted by webchick on October 11, 2009 at 6:55am
9 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | node system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | D7csmtl, Needs tests, TestingPartySF |
Issue Summary
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?)
Comments
#1
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
#2
@D7csmtl
For starters, lets add a pass to check for clickable links via /node
#3
@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.
#4
@D7csmtl
These tests were originally introduced in http://drupal.org/node/557292#comment-2134726
#5
@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)
#6
go testbot go
#7
The last submitted patch, node-601308-2.patch, failed testing.
#8
reroll
#9
#10
@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.
#11
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!
#12
#13
Code sprint tag
#14
Updated the patch based on webchick's comments. The patch has also been rerolled against HEAD.
#15
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.
#16
I tested this, too, and did not encounter any errors.
#17
+1 tested, all tests pass and code style looks good.
#18
Committed to CVS HEAD. Thanks for your contribution! :)
#19
Automatically closed -- issue fixed for 2 weeks with no activity.